-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[DOCS] SQL: Add formal API docs #75506
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
Conversation
|
Pinging @elastic/clients-team (Team:Clients) |
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/es-docs (Team:Docs) |
rest-api-spec/src/main/resources/rest-api-spec/api/sql.clear_cursor.json
Outdated
Show resolved
Hide resolved
sethmlarson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL updates in API specs LGTM with one nit.
bpintea
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, thank you, I've left some minor notes, just one omission that should be corrected.
| `false`. | ||
|
|
||
| `keep_alive`:: | ||
| (Optional, <<time-units,time value>>) Retention period for the search and its |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth mentioning that this is an async search parameter? Not to have the user assume that any result will be stored by simply providing this parameter alone; or at least mention the required tandem with wait_for_completion_timeout.
And maybe same for keep_on_completion and wait_for_completion_timeout[*]; like for id, is_running.
[*] As implementation detail also when attempting to "store a sync search", that search is actually turned async internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out. For the keep_alive parameter, I reworded to:
Retention period for an async or saved synchronous search. Defaults to 5d (five days).
Both the async and saved synchronous search copy have xrefs.
I added similar xrefs to the keep_on_completion and wait_for_completion_timeout parameter definitions.
| `keep_alive`:: | ||
| (Optional, <<time-units,time value>>) Retention period for the search and its | ||
| results. Defaults to `5d` (five days). | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep_on_completion is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this.
| both this parameter and the `Accept` HTTP header, the API only uses this | ||
| parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a proposal: ", this parameter takes precedence."? Your formulation is easy to understand, the proposal might be more established in case of overlapping specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll use:
If you specify both this parameter and the `Accept` HTTP header, this parameter take precedence.
| <<sql-pagination,pagination request>> fails. Defaults to `45s` (45 seconds). | ||
|
|
||
| `params`:: | ||
| (Optional, array) Values for variables in the `query`. For variable syntax, see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (Optional, array) Values for variables in the `query`. For variable syntax, see | |
| (Optional, array) Values for parameters in the `query`. For variable syntax, see |
Some SQL implementations allow for variables in procedural extensions -- I'd avoid this term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out.
| [[clear-sql-cursor-api-limitations]] | ||
| ===== Limitations | ||
|
|
||
| See <<sql-limitations>>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we (currently) have any limitations specifically targeting clear cursor and the async-specific APIs. They are mostly about the SQL implementation and interaction with Elasticsearch concepts. It seems futer-safer to have the limitation section on these pages, but we can also add them if/when we introduce them. Just FYI, in case this section isn't added preventively on all pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. This is largely here for future-proofing. I'll leave it in for now as it's consistent with our EQL docs.
|
Thanks, @bpintea! |
|
@elasticmachine update branch |
bpintea
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Adds formal API docs for the following APIs: * Clear SQL cursor * SQL search * SQL translate Other changes: * Removes and redirects the "Supported REST parameters section." This is now covered in the SQL search API docs. * Updates a few related xrefs. Closes #75085 # Conflicts: # docs/reference/redirects.asciidoc
Adds formal API docs for the following APIs: * Clear SQL cursor * SQL search * SQL translate Other changes: * Removes and redirects the "Supported REST parameters section." This is now covered in the SQL search API docs. * Updates a few related xrefs. Closes #75085 # Conflicts: # docs/reference/redirects.asciidoc
Adds formal API docs for the following APIs: * Clear SQL cursor * SQL search * SQL translate Other changes: * Removes and redirects the "Supported REST parameters section." This is now covered in the SQL search API docs. * Updates a few related xrefs. Closes #75085
Adds formal API docs for the following APIs: * Clear SQL cursor * SQL search * SQL translate Other changes: * Removes and redirects the "Supported REST parameters section." This is now covered in the SQL search API docs. * Updates a few related xrefs. Closes elastic#75085
Adds formal API docs for the following APIs:
Other changes:
Closes #75085
Preview