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

Add scroll to search request serialization. #677

Closed

Conversation

frivard-coveo
Copy link

Description

Add the scroll parameter when serializing the SearchRequest.

Issues Resolved

#676

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.

Signed-off-by: Francois Rivard <frivard@coveo.com>
@reta
Copy link
Collaborator

reta commented Oct 19, 2023

@frivard-coveo could you please add an entry to CHANGELOG.md? thank you

@reta
Copy link
Collaborator

reta commented Oct 19, 2023

@VachaShah @dblock it seems like we don't have test cases for regular search functionality, I will add one shortly

@VachaShah
Copy link
Collaborator

@VachaShah @dblock it seems like we don't have test cases for regular search functionality, I will add one shortly

Thank you @reta! We have some search scenarios in samples as well: https://github.com/opensearch-project/opensearch-java/blob/main/samples/src/main/java/org/opensearch/client/samples/Search.java

Signed-off-by: Francois Rivard <frivard@coveo.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Andriy Redko <drreta@gmail.com>
Signed-off-by: Francois Rivard <74623412+frivard-coveo@users.noreply.github.com>
@dblock
Copy link
Member

dblock commented Oct 20, 2023

This does need a test, let's add those as part of (merging) this PR.

@reta
Copy link
Collaborator

reta commented Oct 20, 2023

This does need a test, let's add those as part of (merging) this PR.

@dblock I've added basic Search API test (#678) so we could add more tests on top

@dblock
Copy link
Member

dblock commented Oct 23, 2023

This does need a test, let's add those as part of (merging) this PR.

@dblock I've added basic Search API test (#678) so we could add more tests on top

Thanks.

For this bug fix however we should make sure that there's a test that would fail without this fix.

@frivard-coveo
Copy link
Author

Oh no!

I just realized that this PR is all wrong. The scroll parameter is supposed to be part of the query path, not the request body.
See this doc : https://opensearch.org/docs/latest/api-reference/search/#url-parameters
Unfortunately this means I'm back to square one trying to understand why my scroll requests are failing with "all shards failed".

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.

4 participants