Guard the auto expand replicas setting against improper values #6401
Conversation
Previously if the user provided a non-conforming string, it would blow up with `java.lang.StringIndexOutOfBoundsException: String index out of range: -1` which is not a *helpful* error message. Also updated the documentation to make the possible setting values more clear.
This was based on feedback from the team
This also switches from `IllegalArgumentException` to `ElasticsearchIllegalArgumentException` to be more consistent with the rest of the file.
| autoExpandReplicas, IndexMetaData.SETTING_AUTO_EXPAND_REPLICAS); | ||
| logger.warn(errorMessage); | ||
| throw new ElasticsearchIllegalArgumentException(errorMessage); | ||
| } |
There was a problem hiding this comment.
I think it would make sense to put all this before the try/catch block and call continue instead of throwing an exception so that the warning doesn't get logged twice?
There was a problem hiding this comment.
Then maybe the try/catch block could only catch a NumberFormatException and complain about wrongly-formatted numbers instead of the vague wrong format message?
|
Thanks @mdaniel this is a nice cleanup! I left one comment inline, let me know what you think. |
It is (hopefully!) not going to change during iteration.
Hopefully this makes it more prominent versus burying it inside the for loop
Based on feedback by @jpountz
This commit does not re-indent the block in order to make the actual change more apparent. I will re-indent the block in the next commit. This was based on feedback by @jpountz
Previously that message was used twice, but now that it is only used in the log, go ahead and use the log formatting mechanism
It seems none of the rest of the log messages use quotes, and use array brackets instead. This just harmonizes things.
|
I have updated the PR according to your comments, and hopefully added some additional sanity along the way. In my initial effort, I was trying not to touch parts of the code that were not directly related to my issue. So I hope I didn't go too far the other way this time. :-) Also, while we're discussing this, if one provides a bogus Is there a mechanism through which we can surface the fact that their setting was not enacted? |
|
Agreed that it is misleading. This method is called in a listener that is not supposed to throw exceptions, so I guess the validation would need to be performed up-front... May I propose to try to tackle it in another change, I think your current PR is already a great improvement! |
|
I can agree with that; is there anything else you need from me for this one? |
|
Thanks for fixing this! I can't be sure but I imagine @jpountz'll want to make sure you've signed the CLA and he might want you to squash the commit. I believe he'll merge it squashed but I can't be sure whether it's best if you squash it or if he does. |
|
CLA is already signed, so all is good! I'll handle the squashing myself. |
This is the PR for Issue #5752
It updates the documentation to be more precise, and adds guards in the code to ensure the values are recognized, and if not it emits a log message and throws an exception with a more helpful message.