Add deprecation warning for negative index.unassigned.node_left.delayed_timeout #26832

DaveCTurner · 2017-09-29T10:14:29Z

There are also usages in o.e.c.routing.allocation.AllocationService and o.e.gateway.ReplicaShardAllocator. Do these also want warnings?

bleskes · 2017-10-04T09:30:06Z

core/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java

@@ -366,6 +370,9 @@ public AllocationStatus getLastAllocationStatus() {
     */
    public long getRemainingDelay(final long nanoTimeNow, final Settings indexSettings) {
        long delayTimeoutNanos = INDEX_DELAYED_NODE_LEFT_TIMEOUT_SETTING.get(indexSettings).nanos();
+        if (delayTimeoutNanos < 0) {


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.

DaveCTurner · 2017-10-04T11:00:56Z

Thanks @bleskes - here's another attempt. Wasn't sure whether to put the check here in Settings or put it in TimeValue instead, and also how much to reuse the existing code vs copy it.

jasontedor

I left some comments.

jasontedor · 2017-10-04T11:08:35Z

core/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -1089,6 +1094,16 @@ private static String arrayToParsableString(String[] array) {
        return timeSetting(key, defaultValue, TimeValue.timeValueMillis(0), properties);
    }

+    public static Setting<TimeValue> timeSettingWithNegativeValuesDeprecated(String key, TimeValue defaultValue, Property... properties) {


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.

DaveCTurner · 2017-10-04T12:40:42Z

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?

jasontedor · 2017-10-04T12:54:08Z

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.

DaveCTurner · 2017-10-04T13:09:27Z

Ok @jasontedor, is this what you meant? Or does this need to be lazy too?

jasontedor · 2017-10-04T13:52:55Z

No, laziness should not be needed here. The reason we need laziness in Settings is because settings class gets initialized before we’ve configured logging and we do not want the deprecation logger to be initialized when the class initializer for Settings runs (note the Javadoc on the holder in the Settings class for more details).

jasontedor

Getting close.

jasontedor · 2017-10-04T13:54:20Z

core/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java

@@ -47,11 +49,20 @@
 */
 public final class UnassignedInfo implements ToXContentFragment, Writeable {

+    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(Setting.class));


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.

Darn it. Fixed.

jasontedor

Can we have a test that exercises this and asserts the warning that we expect happens?

DaveCTurner · 2017-10-05T09:22:09Z

@jasontedor @bleskes Felt like quite an adventure to get those three lines of test, so I hope they're close to what you wanted.

jasontedor

LGTM.

Add deprecation warning in getRemainingDelay()

99ec200

DaveCTurner added :Allocation >deprecation v6.1.0 labels Sep 29, 2017

DaveCTurner requested a review from bleskes September 29, 2017 10:14

bleskes suggested changes Oct 4, 2017

View reviewed changes

DaveCTurner added 5 commits October 4, 2017 11:39

Revert "Add deprecation warning in getRemainingDelay()"

8a312cc

This reverts commit 99ec200. Should be done by changing the parser.

Line too long

75f5fc6

Introduce optional futureMinValue argument

13318ed

Introduce Setting#timeSettingWithNegativeValuesDeprecated() and use i…

37076fb

…t for this setting

Revert "Introduce optional futureMinValue argument"

6711490

Did not mean to commit this - decided on a different design.

jasontedor requested changes Oct 4, 2017

View reviewed changes

DaveCTurner added 3 commits October 4, 2017 13:36

Make DeprecationLoggerHolder public

d94ff25

Inline timeSettingWithNegativeValuesDeprecated()

37bc8a6

Reinstate (unused, but removed-in-error) import

a8c565a

DaveCTurner added 2 commits October 4, 2017 14:03

Line length

b5a2a4d

Use our own DeprecationLogger

6f8269e

jasontedor requested changes Oct 4, 2017

View reviewed changes

Use the logger for the right class this time

3e6fb49

jasontedor requested changes Oct 4, 2017

View reviewed changes

DaveCTurner added 2 commits October 5, 2017 10:07

A little less tautology and a little more action please

9616630

Test that the warning is fired

95d1097

jasontedor approved these changes Oct 5, 2017

View reviewed changes

bleskes approved these changes Oct 9, 2017

View reviewed changes

DaveCTurner merged commit 0ba3afe into elastic:6.x Oct 9, 2017

DaveCTurner deleted the negative_delayed_timeout_deprecation branch October 9, 2017 18:47

DaveCTurner restored the negative_delayed_timeout_deprecation branch November 22, 2017 15:43

lcawl added :Search/Search Search-related issues that do not fall into other categories and removed :Allocation labels Feb 13, 2018

clintongormley added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Search/Search Search-related issues that do not fall into other categories labels Feb 13, 2018

DaveCTurner deleted the negative_delayed_timeout_deprecation branch October 5, 2018 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation warning for negative index.unassigned.node_left.delayed_timeout #26832

Add deprecation warning for negative index.unassigned.node_left.delayed_timeout #26832

DaveCTurner commented Sep 29, 2017

bleskes Oct 4, 2017

DaveCTurner commented Oct 4, 2017

jasontedor left a comment

jasontedor Oct 4, 2017

DaveCTurner commented Oct 4, 2017

jasontedor commented Oct 4, 2017

DaveCTurner commented Oct 4, 2017

jasontedor commented Oct 4, 2017

jasontedor left a comment

jasontedor Oct 4, 2017

DaveCTurner Oct 4, 2017

jasontedor left a comment

DaveCTurner commented Oct 5, 2017

jasontedor left a comment

Add deprecation warning for negative index.unassigned.node_left.delayed_timeout #26832

Add deprecation warning for negative index.unassigned.node_left.delayed_timeout #26832

Conversation

DaveCTurner commented Sep 29, 2017

bleskes Oct 4, 2017

Choose a reason for hiding this comment

DaveCTurner commented Oct 4, 2017

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Oct 4, 2017

Choose a reason for hiding this comment

DaveCTurner commented Oct 4, 2017

jasontedor commented Oct 4, 2017

DaveCTurner commented Oct 4, 2017

jasontedor commented Oct 4, 2017

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Oct 4, 2017

Choose a reason for hiding this comment

DaveCTurner Oct 4, 2017

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 5, 2017

jasontedor left a comment

Choose a reason for hiding this comment