Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document new experimental ingestion streaming APIs #584

Merged
merged 10 commits into from
Sep 30, 2024

Conversation

reta
Copy link
Contributor

@reta reta commented Sep 18, 2024

Description

Document new experimental ingestion streaming APIs

Issues Resolved

Closes #537

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

/_bulk/stream:
post:
operationId: bulk.stream.0
x-operation-group: bulk
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The running linter will throw an error on the operationId and the x-operation-group.
I think you want to do bulk_stream for x-operation-group and bulk_stream.0 for operationId. Then on the generated client, there will be a client.bulk_stream() method, which is different from client.bulk()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the hints, @nhtruong !

@nhtruong
Copy link
Collaborator

Please add tests to these operations as well.

Copy link
Contributor

github-actions bot commented Sep 18, 2024

Changes Analysis

Commit SHA: 8043e59
Comparing To SHA: 7d05664

API Changes

Summary

├─┬Paths
│ ├──[➕] path (340:3)
│ └──[➕] path (9703:3)
└─┬Components
  ├──[➕] requestBodies (25199:7)
  ├──[➕] responses (26808:7)
  ├──[➕] parameters (14377:7)
  ├──[➕] parameters (14433:7)
  ├──[➕] parameters (14426:7)
  ├──[➕] parameters (14385:7)
  ├──[➕] parameters (14370:7)
  ├──[➕] parameters (14404:7)
  ├──[➕] parameters (14349:7)
  ├──[➕] parameters (14356:7)
  ├──[➕] parameters (14363:7)
  ├──[➕] parameters (14341:7)
  ├──[➕] parameters (14395:7)
  ├──[➕] parameters (14412:7)
  ├──[➕] parameters (14419:7)
  └──[➕] schemas (29888:7)

Document Element Total Changes Breaking Changes
paths 2 0
components 16 0
  • Total Changes: 18
  • Additions: 18

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11108235686/artifacts/1996253002

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 25 29 4

Copy link
Contributor

github-actions bot commented Sep 18, 2024

Spec Test Coverage Analysis

Total Tested
504 303 (60.12 %)

Signed-off-by: Andriy Redko <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start. Needs tests, please!

spec/namespaces/_core.yaml Outdated Show resolved Hide resolved
spec/namespaces/_core.yaml Outdated Show resolved Hide resolved
spec/namespaces/_core.yaml Outdated Show resolved Hide resolved
@reta reta requested review from dblock and nhtruong September 19, 2024 12:47
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to update the 2.17 version used to the released one, and switch the next version to 2.18. Also see validator failures, etc.

@@ -0,0 +1,54 @@
$schema: ../../../json_schemas/test_story.schema.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File should be _core/bulk/stream.yaml.

@@ -0,0 +1,60 @@
$schema: ../../../json_schemas/test_story.schema.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, bulk/stream.yaml.

Signed-off-by: Andriy Redko <[email protected]>
@dblock
Copy link
Member

dblock commented Sep 27, 2024

So does it actually stream data? 🤔

@reta
Copy link
Contributor Author

reta commented Sep 27, 2024

So does it actually stream data? 🤔

It does, I am not sure how we could express that in OpenAPI

@dblock
Copy link
Member

dblock commented Sep 27, 2024

So does it actually stream data? 🤔

It does, I am not sure how we could express that in OpenAPI

I don't think we need to, but do clients automatically do it? AFAIK we don't have code in our tooling that does anything special.

@reta
Copy link
Contributor Author

reta commented Sep 27, 2024

ERROR   _core/bulk/stream.yaml (tests/default/_core/bulk/stream.yaml)
    ERROR   CHAPTERS
        ERROR   Create an index.
            PASSED  REQUEST BODY
            ERROR   RESPONSE STATUS (Expected status 200, but received 500: application/json. The engine does not support HTTP streaming, unable to serve uri [/_bulk/stream] and method [POST])
            SKIPPED RESPONSE PAYLOAD BODY
            SKIPPED RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES
        SKIPPED Delete document in an index.
        SKIPPED Bulk document CRUD.

@dblock need your advice here: streaming (as of today) requires dedicated transport plugin. Here are 2 options we could make it work:

  • ignore the test if particular plugin is not installed, or
  • install (and make default) the streaming transport

@reta
Copy link
Contributor Author

reta commented Sep 27, 2024

I don't think we need to, but do clients automatically do it? AFAIK we don't have code in our tooling that does anything special.

Oh, we don't need to actually, no special treatment required

@dblock
Copy link
Member

dblock commented Sep 27, 2024

  • install (and make default) the streaming transport

I think this is the right answer. The tooling in this repo should support streaming just like we support various content types.

@@ -0,0 +1,60 @@
$schema: ../../../../json_schemas/test_story.schema.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll have to move tests in the plugins folder as well from tests/default.

@dblock dblock merged commit ee2d99c into opensearch-project:main Sep 30, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Document new experimental ingestion streaming APIs
3 participants