Standardize int, long, double and float Setting constructors. #665
Conversation
Signed-off-by: dblock <[email protected]>
dblock
force-pushed
the
settings-constructors
branch
from
d1fe9f9 to
6afba62
Compare
|
start gradle check |
|
❌ Gradle Precommit failure d1fe9f917da84a0f9db3753c6b1c4ee2eb0c2392 |
|
❌ Gradle Wrapper Validation failure d1fe9f917da84a0f9db3753c6b1c4ee2eb0c2392 :alert: Gradle Wrapper integrity has been altered |
|
❌ DCO Check Failed d1fe9f917da84a0f9db3753c6b1c4ee2eb0c2392 |
|
✅ DCO Check Passed 6afba62 |
|
✅ Gradle Wrapper Validation success 6afba62 |
Signed-off-by: dblock <[email protected]>
|
✅ DCO Check Passed 50d320f |
|
✅ Gradle Wrapper Validation success 50d320f |
| } | ||
|
|
||
| public static Setting<Float> floatSetting(String key, float defaultValue, float minValue, Property... properties) { | ||
| return new Setting<>(key, (s) -> Float.toString(defaultValue), (s) -> { |
There was a problem hiding this comment.
The new parser checks min and max, and we set max to Float.MAX_VALUE here, this works. I can do something more elaborate with separate parsers, but I don't think it's necessary.
| @@ -106,8 +106,7 @@ | |||
| public static final Setting.AffixSetting<TimeValue> SCRIPT_CACHE_EXPIRE_SETTING = | |||
| Setting.affixKeySetting(CONTEXT_PREFIX, | |||
| "cache_expire", | |||
| key -> Setting.positiveTimeSetting(key, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, TimeValue.timeValueMillis(0), | |||
There was a problem hiding this comment.
This is positive, so >= 0 is redundant. Note that this is probably a confused declaration thinking 0 is the default (it's actually min), as this setting has a fallback, and defaults are used from the fallback.
| @@ -100,7 +100,6 @@ | |||
| Setting.positiveTimeSetting( | |||
| "cluster.remote.initial_connect_timeout", | |||
| SEARCH_REMOTE_INITIAL_CONNECTION_TIMEOUT_SETTING, // the default needs to be thirty seconds when fallback is removed | |||
| TimeValue.timeValueSeconds(30), | |||
There was a problem hiding this comment.
This says "the value of this setting needs to be at least 30" rather than "the default value is 30", so this looks like a bug to me.
Signed-off-by: dblock <[email protected]>
|
✅ Gradle Wrapper Validation success 44a7f57 |
|
✅ DCO Check Passed 44a7f57 |
|
✅ Gradle Precommit success 44a7f57 |
|
start gradle check |
|
✅ DCO Check Passed 33102173951b457a69f30b5cd2c9639c8a734ae7 |
|
✅ Gradle Wrapper Validation success 33102173951b457a69f30b5cd2c9639c8a734ae7 |
Signed-off-by: dblock <[email protected]>
dblock
force-pushed
the
settings-constructors
branch
from
3310217 to
870127e
Compare
|
✅ DCO Check Passed 870127e |
|
❌ Gradle Precommit failure 33102173951b457a69f30b5cd2c9639c8a734ae7 |
|
✅ Gradle Wrapper Validation success 870127e |
| if (value < minValue) { | ||
| String err = "Failed to parse value" + (isFiltered ? "" : " [" + s + "]") + " for setting [" + key + "] must be >= " + minValue; | ||
| throw new IllegalArgumentException(err); | ||
| } | ||
| if (value > maxValue) { | ||
| String err = "Failed to parse value" + (isFiltered ? "" : " [" + s + "]") + " for setting [" + key + "] must be <= " + maxValue; | ||
| throw new IllegalArgumentException(err); | ||
| } |
There was a problem hiding this comment.
I think it would be better to phrase the error message to include both the min and max:
Failed to parse value: <key> must in in the range [<min>, <max>]
It's a better user experience when all the constraints are spelled out. Also, less duplication in this code.
There was a problem hiding this comment.
This is basically existing code that was moved. The problem with the suggestion is that often max is MAX_INT or MAX_LONG, so in those cases you don't want this. Leaving it as an exercise for another time.
|
✅ Gradle Precommit success 870127e |
|
start gradle check |
…#665) Signed-off-by: Vacha Shah <[email protected]>
Signed-off-by: dblock [email protected]
Description
Enables an alterative to #643.
Refactored and corrected inconsistencies between constructors of various types (int, float, double and long). This adds some missing constructors. (There are two ways to create settings: (a) default value, min, max, etc., and (b) fallback value, min, max, etc. Each type now has a single constructor that calls
new Setting<>for the fallback version, and a single constructor that callsnew Setting<>for the default version. All other constructors call these two.)Corrected a bug where the min value is confused with default value for
positiveTimeSettingused together with a fallback. I removed that constructor to catch any instances (found one).Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.