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

[Docs] Document Scroll API for Java High Level REST Client #25554

Merged
merged 3 commits into from
Jul 7, 2017

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Jul 5, 2017

This PR adds documentation for both search and clear scroll APIs in the high level REST client.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a few comments, LGTM otherwise

// end::search-scroll-id

// tag::search-scroll-execute
SearchResponse searchResponse = client.searchScroll(scrollRequest); // <1>
Copy link
Member

Choose a reason for hiding this comment

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

shall we have the scroll execute in a loop instead? for both sync and async, so we have a complete example on how users are supposed to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed things a bit to use a loop in the main example (the sync / async parts remain the same) as I find it useful and easy to understand. But it gets more complex using async and the boilerplate code make things harder to read.

I think I changed to a good compromise, let me know what you think

--------------------------------------------------
include-tagged::{doc-tests}/SearchDocumentationIT.java[scroll-request-scroll]
--------------------------------------------------
<1> Scroll value (ie, the time to keep alive the search context) as a `TimeValue`
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that the scroll was optional. Maybe we should mention the default value when not specified?


The search contexts used by the Scroll API are automatically deleted when the scroll
times out. But it is advised to release search contexts as soon as they are not
necessary anymore using the Clear Scroll API:
Copy link
Member

Choose a reason for hiding this comment

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

Replace : with . ?

==== Providing the scroll identifiers
The `ClearScrollRequest` allows to clear one or more scroll identifiers in a single request.

The scroll identifier can be added to the request one by one:
Copy link
Member

Choose a reason for hiding this comment

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

identifiers

--------------------------------------------------
include-tagged::{doc-tests}/SearchDocumentationIT.java[clear-scroll-response]
--------------------------------------------------
<1> Return true if the request succeed
Copy link
Member

Choose a reason for hiding this comment

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

succeeded? or succeeds ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed to succeeded

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I do like the idea of having a complete example for scrolling where we loop but this is lovely to have even without that.

SearchResponse searchResponse = client.searchScroll(scrollRequest); // <1>
SearchHits searchHits = searchResponse.getHits(); // <2>
String nextScrollId = searchResponse.getScrollId(); // <3>
while (true) {
Copy link
Member

Choose a reason for hiding this comment

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

how about having a complete example with the initial search response too? Maybe leave a separate search scroll snippet, then move to the complete example where you reuse the SearchHits from the initial SearchResponse and you could do while(searchHits != null && searchHits.length > 0) instead of while(true)? Would that work or would it be too complicated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I updated the doc in 0c2d815, let me know if this is OK or not.

I'm personally fine with the latest version as well as the previous one too, just let me know which one seems better to you.

@javanna javanna merged commit b06a744 into elastic:master Jul 7, 2017
javanna pushed a commit that referenced this pull request Jul 7, 2017
This commit adds documentation for _search/scroll and clear scroll methods of the high level Java REST client
@tlrx
Copy link
Member Author

tlrx commented Jul 7, 2017

Thanks @javanna

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 7, 2017
* master: (42 commits)
  Harden global checkpoint tracker
  Remove deprecated created and found from index, delete and bulk (elastic#25516)
  fix testEnsureVersionCompatibility for 5.5.0 release
  fix Version.v6_0_0 min compatibility version to 5.5.0
  Add bwc indices for 5.5.0
  Add v5_5_1 constant
  [DOCS] revise high level client Search Scroll API docs (elastic#25599)
  Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437)
  Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254)
  [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575)
  Enable cross-setting validation
  [Docs] Fix typo in bootstrap-checks.asciidoc (elastic#25597)
  Index ids in binary form. (elastic#25352)
  bwc checkout should fetch from all remotes
  IndexingIT should check for global checkpoints regardless of master version
  [Tests] Add tests for PhraseSuggestionBuilder#build() (elastic#25571)
  Remove unused class MinimalMap (elastic#25590)
  [Docs] Document Scroll API for Java High Level REST Client (elastic#25554)
  Disable date field mapping changing (elastic#25285)
  Allow BWC Testing against a specific branch (elastic#25510)
  ...
@tlrx tlrx deleted the scroll-docs branch July 24, 2017 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants