BUG: Negative value is successfully set for “discovery.zen.commit_timeout” parameter

[nikhilkapoornk]

I am able to successfully set the negative value of "discovery.zen.commit_timeout" parameter in elasticsearch.yml or when it's value is changed dynamically by using the cluster update settings API.

Whereas an error is shown on the console when "discovery.zen.publish_timeout" parameter is set to negative value.
{Failed to parse value [-10s] for setting [discovery.zen.publish_timeout] must be >= 0s (Debug Log)}

Elasticsearch Version: 5.5 & 5.4.3

java -version

openjdk version "1.8.0_171"
OpenJDK Runtime Environment (build 1.8.0_171-b10)
OpenJDK 64-Bit Server VM (build 25.171-b10, mixed mode)

Regards
Nikhil Kapoor

[elasticmachine]

Pinging @elastic/es-distributed

[DaveCTurner]

These parameters are going away in 7.0 (cf #32006) so I don't think it's worth putting any effort into extra validation in the remainder of the 6.x series. However I think we should document that a negative value for discovery.zen.commit_timeout means the same as zero.

[jn01674361]

Hi! Can I work on this issue? :)

[DaveCTurner]

@jn01674361 sure, go ahead.

[taraka-vishnumolakala]

@jn01674361 are you still working on this issue ?

[jn01674361]

@swrp I plan to but I have not gotten around to starting yet, unfortunately. Why do you ask?

[cshtdd]

I created PR #37570 to document discovery.zen.commit_timeout treats negative values as 0.

[andrershov]

Volunteers are still welcome

[harshbajaj16]

Hi @andrershov ,

Yes, I would like to work on this.

Br,
Harsh Bajaj

[harshbajaj16]

Hi @andrershov and @DaveCTurner ,

I've created a PR for this bug in 6.7 branch. could you please verify the same?

Regards,
Harsh Bajaj

[DaveCTurner]

Thanks @harshbajaj16, but this is a docs issue as I mentioned above:

However I think we should document that a negative value for discovery.zen.commit_timeout means the same as zero.

We don't want to change the behaviour here -- it's technically a breaking change which we try and avoid doing in minor releases.

[harshbajaj16]

Hi @DaveCTurner ,

Thank you for your reply!

I want to further clarify few things.
I have check the validation for similar parameter "discovery.zen.publish_timeout", negative handling (less than 0ms) is present.

Code Reference:
File: server/src/main/java/org/elasticsearch/common/settings/Setting.java
Method: public static Setting timeSetting(
Source code:

  TimeValue timeValue = TimeValue.parseTimeValue(s, null, key);
                if (timeValue.millis() < minValue.millis()) {
                    throw new IllegalArgumentException("Failed to parse value [" + s + "] for setting [" + key + "] must be >= " + minValue);
                }
               return timeValue;

---

Steps to verify:

1. Settings through configuration
   Step 1:
   Set any negative value, say discovery.zen.publish_timeout=-10 in elasticsearch.yml
   Step 2:
   Start elasticsearch
   # systemctl start elasticsearch

2. Setting through Cluster Update API
   # curl -H "Content-Type: application/json" -XPUT localhost:9200/_cluster/settings -d "{"transient" : {"discovery.zen.publish_timeout" : "-10s"}}"

Observation:
It is throwing below illegal argument exception while setting any negative value, as mentioned in above steps:
org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: failed to parse value [-10s] for setting [discovery.zen.publish_timeout], must be >= [0ms]

Based on above existing behaviour for discovery.zen.publish_timeout, we have proposed the simiar fix for “discovery.zen.commit_timeout”.

Code Reference (Patch Fix):

diff --git a/server/src/main/java/org/elasticsearch/discovery/DiscoverySettings.java b/server/src/main/java/org/elasticsearch/discovery/DiscoverySettings.java
index 8cb2f6c..88b803f 100644
--- a/server/src/main/java/org/elasticsearch/discovery/DiscoverySettings.java
+++ b/server/src/main/java/org/elasticsearch/discovery/DiscoverySettings.java
@@ -54,7 +54,14 @@ public class DiscoverySettings {
      */
     public static final Setting<TimeValue> COMMIT_TIMEOUT_SETTING =
         new Setting<>("discovery.zen.commit_timeout", (s) -> PUBLISH_TIMEOUT_SETTING.getRaw(s),
-            (s) -> TimeValue.parseTimeValue(s, TimeValue.timeValueSeconds(30), "discovery.zen.commit_timeout"),
+            (s) -> {
+                TimeValue timeValue = TimeValue.parseTimeValue(s, TimeValue.timeValueSeconds(30), "discovery.zen.commit_timeout");
+                if (timeValue.millis() < 0) {
+                    throw new IllegalArgumentException("Failed to parse value [" + s + "] for setting [discovery.zen.commit_timeout]," +
+                                                       " must be >= [0ms]");
+                }
+                return timeValue;
+            },
             Property.Dynamic, Property.NodeScope);
     public static final Setting<ClusterBlock> NO_MASTER_BLOCK_SETTING =
         new Setting<>("discovery.zen.no_master_block", "write", DiscoverySettings::parseNoMasterBlock,

Steps to verify:

1. Settings through configuration
   Step 1:
   Set any negative value, say discovery.zen.commit_timeout=-10 in elasticsearch.yml
   Step 2:
   Start elasticsearch
   # systemctl start elasticsearch

2. Setting through Cluster Update API
   # curl -H "Content-Type: application/json" -XPUT localhost:9200/_cluster/settings -d "{"transient" : {"discovery.zen.commit_timeout" : "-10s"}}"

Observation:
After the proposed fix, now It is throwing below illegal argument exception while setting any negative value, as mentioned in above steps:
org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: failed to parse value [-10s] for setting [discovery.zen.commit_timeout], must be >= [0ms]

Conclusion:
We did proper sanity fox this fix and performed rigorous UT covering all the possible aspects.
Exceptional Handling in case of setting any negative value for discovery.zen.commit_timeout is working perfectly.
This fix will provide extra level of validation for parameter discovery.zen.commit_timeout.

Please share your opinion for above discussed scenario.

Best Regards,
Harsh Bajaj

[DaveCTurner]

After the proposed fix, now It is throwing below illegal argument exception while setting any negative value, as mentioned in above steps:

This is an unnecessary breaking change and is not acceptable.

[harshbajaj16]

Hi,
Thanks for your confirmation.
Can you please explain "breaking changes" in brief, for our understanding.

Best Regards,
Harsh Bajaj

[DaveCTurner]

Sure. It's a change that would break some users' configuration when they upgrade from a version without your proposed change to a version with your proposed change. Elasticsearch supports upgrades within the same major series, so users should be able to upgrade from any 6.x version to any newer 6.x version. However if someone had (for some unknown reason) set

discovery.zen.commit_timeout: -10s

in their elasticsearch.yml file then your change would prevent them from upgrading: in 6.7.0, say, this setting would work fine (and mean the same as discovery.zen.commit_timeout: 0s) whereas in a version with your change it would throw an error and the node would refuse to start. This change in behaviour broke their configuration, hence, a breaking change.

We do sometimes make such changes when it's absolutely necessary to do so, but in this case it isn't necessary, so we should avoid doing it.

[harshbajaj16]

Hi @DaveCTurner ,

Thank you.

Could you please review the new pull request opened for this issue "document update" #42834 ?

Regards,
Harsh Bajaj

[DaveCTurner]

Closed by #42834.