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
Return missing (404) if a scroll_id is cleared that no longer exists. #5865
Return missing (404) if a scroll_id is cleared that no longer exists. #5865
Conversation
this.succeeded = succeeded; | ||
this.noneFreed = noneFreed; |
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 like the name too much maybe we can just count the number of freed contexts and return that responding with 404
if it's == 0
? then we can call it numFreed
?
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.
sure make sense, I'll update the PR
@s1monw I updated the PR to replace noneFreed to numFreed, so now it just counts the number of freed search contexts. |
Updated PR to latest master. |
* @return The number of seach contexts that were freed. If this is <code>0</code> the assumption can be made, | ||
* that the scroll id specified in the request did not exist. (never existed, was expired, or completely consumed) | ||
*/ | ||
public int isNumFreed() { |
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.
more likely getNumFreed()
as it is not a boolean?
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'll change that :) (used to be a boolean property: freed
)
left two comments. One last thing though that bothers me: In |
Thanks for reviewing it! The change of the type in |
Let ClearScrollResponse implement StatusToXContent
@spinscale I updated the PR and addressed the two comments. |
LGTM |
Pushed to master and 1.x branches. |
PR for #5730