Skip to content

Conversation

@cbuescher
Copy link
Member

Initial documentation of high level rest client search request and response.
This mostly covers regular search requests and responses and doesn't include
aggregations, suggestions and highlighting just yet.

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.

I left a few comments on how I would structure it, it's mainly a matter of moving things around a bit but the initial content is fine.

Copy link
Member

Choose a reason for hiding this comment

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

documents can also be retrieved through the get api ;) maybe slightly rephrase?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe explain that SearchSourceBuilder allows to set whatever goes into the request body? At least to explain why we have two objects, the request and the source.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to follow other APIs docs and first talk about the request and all its options, then have a section about the response and what to do with it

Copy link
Member

Choose a reason for hiding this comment

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

everything is optional here right? so maybe remove the Optional: part?

Copy link
Member

Choose a reason for hiding this comment

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

rather than having a big snippet with 5 or more notes, in other docs we did one snippet per option, it is a bit more work but maybe better when visualized and more readable?

Initial documentation of high level rest client search request and response.
This mostly covers regular search requests and responses and doesn't include
aggregations, suggestions and highlighting just yet.
@cbuescher cbuescher force-pushed the addSearchDocs branch 2 times, most recently from 71b0df1 to adedc99 Compare July 12, 2017 09:22
@cbuescher
Copy link
Member Author

@javanna thanks, I addressed the first comments and reformatted the "optional Request parameters" section according to your last suggestion. However, I'm unsure about where to better use a longer code snippet and some notes, of where its better to have shorter snippets like you suggested. If you think we should do this everywhere I can rework the remaining parts before I merge this.

Copy link
Member

Choose a reason for hiding this comment

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

what is the difference between this example and the previous one? I would maybe merge the two.

Copy link
Member

Choose a reason for hiding this comment

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

do it here like we did it in other pages with the reference in the snippet, that gets visualized after it, rather than the text before it?

Copy link
Member

Choose a reason for hiding this comment

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

call it Optional arguments for consistency with other pages?

Copy link
Member

Choose a reason for hiding this comment

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

between request and response, can you add the Synchronous execution and Asynchronous execution like in the other doc pages?

Copy link
Member

Choose a reason for hiding this comment

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

clarify which one is the index in the example?

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 more comments, LGTM though

@cbuescher cbuescher merged commit f3e7a1c into elastic:master Jul 12, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 12, 2017
* master:
  Fix inadvertent rename of systemd tests
  Adding basic search request documentation for high level client (elastic#25651)
  Disallow lang to be used with Stored Scripts (elastic#25610)
  Fix typo in ScriptDocValues deprecation warnings (elastic#25672)
  Changes DocValueFieldsFetchSubPhase to reuse doc values iterators for multiple hits (elastic#25644)
  Query range fields by doc values when they are expected to be more efficient than points.
  Remove SearchHit#internalHits (elastic#25653)
  [DOCS] Reorganized the highlighting topic so it's less confusing.
jasontedor added a commit to fred84/elasticsearch that referenced this pull request Jul 12, 2017
* master: (181 commits)
  Use a non default port range in MockTransportService
  Add a shard filter search phase to pre-filter shards based on query rewriting (elastic#25658)
  Prevent excessive disk consumption by log files
  Migrate RestHttpResponseHeadersIT to ESRestTestCase (elastic#25675)
  Use config directory to find jvm.options
  Fix inadvertent rename of systemd tests
  Adding basic search request documentation for high level client (elastic#25651)
  Disallow lang to be used with Stored Scripts (elastic#25610)
  Fix typo in ScriptDocValues deprecation warnings (elastic#25672)
  Changes DocValueFieldsFetchSubPhase to reuse doc values iterators for multiple hits (elastic#25644)
  Query range fields by doc values when they are expected to be more efficient than points.
  Remove SearchHit#internalHits (elastic#25653)
  [DOCS] Reorganized the highlighting topic so it's less confusing.
  Add an underscore to flood stage setting
  Avoid failing install if system-sysctl is masked
  Add another parent value option to join documentation (elastic#25609)
  Ensure we rewrite common queries to `match_none` if possible (elastic#25650)
  Remove reference to field-stats docs.
  Optimize the order of bytes in uuids for better compression. (elastic#24615)
  Fix BytesReferenceStreamInput#skip with offset (elastic#25634)
  ...
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.

3 participants