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

Low-level APIs #211

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Low-level APIs #211

merged 1 commit into from
Dec 11, 2023

Conversation

nhtruong
Copy link
Collaborator

  • Generator for low-level methods
  • Generated low-level methods
  • Guide for low-level methods
  • Samples for low-level methods

Issues Resolved

closes #209

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.

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.

I would not document the low level client at all, specifically perform_request. Why would one ever want to use that whereas the .http DSL is way nicer and hides the implementation details.

In terms of guides I would align naming with other libraries, e.g. https://github.com/opensearch-project/opensearch-py/blob/main/guides/json.md

You should have a working sample for this.

See other suggestions, pick and choose.

CHANGELOG.md Outdated Show resolved Hide resolved
api_generator/lib/api_generator.rb Outdated Show resolved Hide resolved
api_generator/lib/api_generator.rb Outdated Show resolved Hide resolved
api_generator/lib/api_generator.rb Show resolved Hide resolved
guides/low_level_api.md Outdated Show resolved Hide resolved
@nhtruong
Copy link
Collaborator Author

nhtruong commented Nov 15, 2023

I would not document the low level client at all, specifically perform_request. Why would one ever want to use that whereas the .http DSL is way nicer and hides the implementation details.

In terms of guides I would align naming with other libraries, e.g. https://github.com/opensearch-project/opensearch-py/blob/main/guides/json.md

You should have a working sample for this.

See other suggestions, pick and choose.

The perform_request method returns an OpenSearch::Transport::Transport::Response object which includes response headers and status on top of the response body, while the http methods only returns the body. Users wanting to access the response headers and status, though rare, will have to resort to using perform_request. I will put the guide http first and add explanation of when to use perform_request (Also it's the first bullet point in #209)

I've already added samples for both approaches :)

json.md isn't as self-explanatory as low-level-api.md, in my opinion.

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.

This looks good.

Before we merge, I have some concerns calling this "low level". In OpenSearch Java client this is not the meaning at all - a low level API comes form OpenSearch core, while a high level API comes from the client. In the Python client we called this "raw JSON", https://github.com/opensearch-project/opensearch-py/blob/main/guides/json.md.

Maybe this is just an API to make "raw HTTP requests"? Does "low" add a lot of meaning?

Having a similar discussion in opensearch-project/opensearch-js#649 as you can see ;)

WDYT?

@nhtruong
Copy link
Collaborator Author

@dblock That makes sense. I've updated the PR according to make it consistent with other clients.

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.

Can we rename the code and file names and mentions of low_level to http as well? It feels like some kind of class of APIs, but really this is just the 6 HTTP methods. Why call it another name?

CHANGELOG.md Outdated Show resolved Hide resolved
api_generator/USER_GUIDE.md Outdated Show resolved Hide resolved
api_generator/USER_GUIDE.md Outdated Show resolved Hide resolved
- Generator for low-level methods
- Generated low-level methods
- Guide for low-level methods
- Samples for low-level methods

Signed-off-by: Theo Truong <theotr@amazon.com>
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.

Looks great!

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

LGTM!

@nhtruong nhtruong merged commit b12a6a7 into opensearch-project:main Dec 11, 2023
56 checks passed
@nhtruong nhtruong deleted the low_level_api branch December 11, 2023 19:00
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] Make raw JSON REST requests to OpenSearch
3 participants