-
Notifications
You must be signed in to change notification settings - Fork 114
Add max_concurrent_shard_requests to open PIT API #4142
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
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
max_concurrent_shard_requests?: long | ||
max_concurrent_shard_requests?: integer |
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 noticed we used long
by mistake here, while it's an int: https://github.com/elastic/elasticsearch/blob/36c14bf3a56f85177f5ce60b2f839b6b35236bdc/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java#L88.
max_concurrent_searches?: long | ||
max_concurrent_searches?: integer |
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.
include_named_queries_score?: boolean | ||
/** | ||
* Maximum number of concurrent searches the multi search API can execute. | ||
* Defaults to `max(1, (# of data nodes * min(search thread pool size, 10)))`. |
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.
Can't use @server_default
as it expects a number
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!
While long
-> int
is technically a breaking change, nobody would notice in the end (if somebody is already passing values > MAXINT to that field, this means it probably never worked anyways).
After discussing this offline, I extracted the breaking changes to #4171. Please take another look. (I did not intend to close this, sorry.) |
Following you can find the validation results for the API you have changed.
You can validate this API yourself by using the |
1 similar comment
Following you can find the validation results for the API you have changed.
You can validate this API yourself by using the |
(cherry picked from commit 6138673)
(cherry picked from commit 6138673)
(cherry picked from commit 6138673)
Kibana ran into this missing parameter: https://github.com/elastic/kibana/pull/213375/files#diff-a742bab88e72c2e7784c7fdfba52ea4c1f45c0939854beaf400a5fd191a92b31R56
It was added elastic/elasticsearch#96782 and is defined in https://github.com/elastic/elasticsearch/blob/36c14bf3a56f85177f5ce60b2f839b6b35236bdc/server/src/main/java/org/elasticsearch/action/search/RestOpenPointInTimeAction.java#L52-L58
I can leave out the long -> integer changes if they are going to break statically typed clients. Or not backport all the way to 8.18.