-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Warmers delete _all should not throw exception when no warmers #13058
Conversation
@@ -92,6 +92,7 @@ public ClusterState execute(ClusterState currentState) { | |||
MetaData.Builder mdBuilder = MetaData.builder(currentState.metaData()); | |||
|
|||
boolean globalFoundAtLeastOne = false; | |||
boolean deleteAll = Arrays.asList(request.names()).contains("_all"); |
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 we use the MetaData.ALL
constant here? and maybe just iterate over the array instead of creating a list just for that?
Thanks for the PR @andrestc I reviewed it and left two comments, would it be possible also to write a test around this change please? |
@javanna Thanks for the review. Will work on it! |
@javanna I've updated the PR with your observations. |
boolean deleteAll = false; | ||
for (int i=0; i<request.names().length; i++){ | ||
if (request.names()[i].equals(MetaData.ALL)) { | ||
deleteAll = true; |
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.
mmm you need a break here otherwise deleteAll will be true only if the last index name is _all
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 thats needed, but I'll add it because we can leave the loop as soon as we find it.
Thanks @andrestc I did another round and left a couple of minor comments, looks good though. |
@javanna thanks! I will work on them today and submit it again later. |
@javanna I've updated the PR. Thanks! |
LGTM thanks @andrestc I will merge this soon.
you test is fine, thanks! |
…egistered Closes elastic#8991 Closes elastic#13058
…egistered Closes elastic#8991 Closes elastic#13058
I think this fixes #8991. It will not throw exceptions when the request is to delete "_all" warmers but log it.