Add deprecation warning for negative index.unassigned.node_left.delayed_timeout#26832
Conversation
| */ | ||
| public long getRemainingDelay(final long nanoTimeNow, final Settings indexSettings) { | ||
| long delayTimeoutNanos = INDEX_DELAYED_NODE_LEFT_TIMEOUT_SETTING.get(indexSettings).nanos(); | ||
| if (delayTimeoutNanos < 0) { |
There was a problem hiding this comment.
instead of baking it here, I would put in the setting parser. This means you'll catch all the places where the setting is used in one place.
This reverts commit 99ec200. Should be done by changing the parser.
…t for this setting
Did not mean to commit this - decided on a different design.
|
Thanks @bleskes - here's another attempt. Wasn't sure whether to put the check here in |
| return timeSetting(key, defaultValue, TimeValue.timeValueMillis(0), properties); | ||
| } | ||
|
|
||
| public static Setting<TimeValue> timeSettingWithNegativeValuesDeprecated(String key, TimeValue defaultValue, Property... properties) { |
There was a problem hiding this comment.
I prefer not adding abstractions with exactly one use case. Especially since when we turn around and remove the deprecation logging for this setting (and turn it into a hard failure with Setting#positiveTimeSetting), the abstraction might end up left behind.
This one is also dangerous because it adds a static logger to a class that is touched very early. If we are not careful, we start initializing logging before we have configured it which we actively try to detect and fail startup if it occurs.
I think that you can get by with manually defining the logic for this setting at the definition site.
|
Thanks @jasontedor - still fumbling towards the right answer here. I inlined the one-use method, and @bleskes showed me how deprecation of an entire setting happens, using a lazily-initialised logger. That logger was package-accessible so I had to make it public to use it here, which makes me uncomfortable. Any better ideas? |
|
Let’s not do that. It’s not the setting that is deprecated here, only support for certain values. So we shouldn’t hit the settings deprecation logger, instead I think a deprecation logger specific to this setting is fine. |
|
Ok @jasontedor, is this what you meant? Or does this need to be lazy too? |
|
No, laziness should not be needed here. The reason we need laziness in |
| */ | ||
| public final class UnassignedInfo implements ToXContentFragment, Writeable { | ||
|
|
||
| private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(Setting.class)); |
There was a problem hiding this comment.
Again, this doesn't need to be tied to the Setting logger, we are not deprecating a setting here, only support for certain values. Therefore, I think this should be the logger for UnassignedInfo.class.
jasontedor
left a comment
There was a problem hiding this comment.
Can we have a test that exercises this and asserts the warning that we expect happens?
|
@jasontedor @bleskes Felt like quite an adventure to get those three lines of test, so I hope they're close to what you wanted. |
cf #26828.
There are also usages in
o.e.c.routing.allocation.AllocationServiceando.e.gateway.ReplicaShardAllocator. Do these also want warnings?