-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[data.search] Add extend functionality to server-side search service #86195
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
@elasticmachine merge upstream |
if (!strategy.extend) { | ||
throw new Error(`Search strategy ${options.strategy} does not support extend`); | ||
} | ||
return strategy.extend(id, keepAlive, options, deps); |
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.
Thoughts about validating the keepAlive
string? It looks like it should be a valid time units string: https://www.elastic.co/guide/en/elasticsearch/reference/current/common-options.html#time-units
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.
After giving this some thought, I think we can rely on the error/message we get back from Elasticsearch for this:
{
"error": {
"root_cause": [
{
"type": "illegal_argument_exception",
"reason": "failed to parse setting [keep_alive] with value [foobar] as a time value: unit is missing or unrecognized"
}
],
"type": "illegal_argument_exception",
"reason": "failed to parse setting [keep_alive] with value [foobar] as a time value: unit is missing or unrecognized"
},
"status": 400
}
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.
The code LGTM but I'm not sure how to test the changes
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.
Cound you also add an api integration test for this API?
test/api_integration/apis/search/search.ts
This PR doesn't add any endpoints to access the |
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
…service (elastic#86195) * [data.search.session] Store search strategy in saved object * [data.search.session] Add extend functionality * Update docs * Update unit test to check strategy * Throw kbnServerError instead of error * Fix test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…service (#86195) (#87320) * [data.search.session] Store search strategy in saved object * [data.search.session] Add extend functionality * Update docs * Update unit test to check strategy * Throw kbnServerError instead of error * Fix test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Build SucceededMetrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
Summary
Adds a third method,
extend
to search strategies. The purpose of this method is to extend the TTL of the given search ID by the given duration (orkeep_alive
).Checklist
For maintainers