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
Fix errors reported by error-prone #9817
Conversation
I compiled elasticsearch with error-prone (https://github.com/google/error-prone). This commit fixes the 13 errors reported by the tool.
HI @sebastianmonte thanks, looks good! Could you please sign our CLA so we can pull this in? |
Hi, thanks for the feedback. Actually I have already signed it (after the pull request). Should I do it again? |
Hi @sebastianmonte I am sorry, maybe something went wrong, but I can't find your signed CLA. Would you mind trying again? |
Hi @javanna , I signed the agreement few minutes ago. |
Thanks @sebastianmonte merged! |
@@ -176,7 +176,7 @@ public ZenDiscovery(Settings settings, ClusterName clusterName, ThreadPool threa | |||
this.rejoinOnMasterGone = settings.getAsBoolean(SETTING_REJOIN_ON_MASTER_GONE, true); | |||
|
|||
if (this.joinRetryAttempts < 1) { | |||
throw new ElasticsearchIllegalArgumentException("'" + SETTING_JOIN_RETRY_ATTEMPTS + "' must be a positive number. got [" + this.SETTING_JOIN_RETRY_ATTEMPTS + "]"); | |||
throw new ElasticsearchIllegalArgumentException("'" + SETTING_JOIN_RETRY_ATTEMPTS + "' must be a positive number. got [" + SETTING_JOIN_RETRY_ATTEMPTS + "]"); |
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.
Shouldn't this be this.joinRetryAttempts
in the message instead of printing the same constant twice?
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.
yes I think this is an oversight. Do you want to send a PR to fix it?
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.
Hmm, I don't know anything about ElasticSearch, don't even use it – I just stumbled over this pull request as a showcase of error-prone. I don't think I have the time to set up my environment to do any proper PR or to test a change like this, sorry.
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.
no worries, thanks for catching this @ePaul !
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 had a look and the settings code has been dramatically improved with 5.0 already, hence this fix is not required any longer. Nothing to do then, but thanks again for pointing this out.
I compiled elasticsearch with error-prone
(https://github.com/google/error-prone) and it reported
13 errors. This commit fixes those errors.
The errors were related to the following: