Fix setting notification for complex setting (affixMap settings) that could cause transient settings to be ignored

[dakrone]

Previously if an affixMap setting was registered, and then a completely
different setting was applied, the affixMap update consumer would be notified
with an empty map. This caused settings that were previously set to be unset in
local state in a consumer that assumed it would only be called when the affixMap
setting was changed.

This commit changes the behavior so if a prefix foo. is registered, any
setting under the prefix will have the update consumer notified if there are
changes starting with foo..

Resolves #28316

[dakrone]

There might be a better way to solve this, which is why I am marking this as "discuss"

[s1monw]

@dakrone it's unclear to me what you are fixing here. is it possible to have a simple unittest that fails previously and passes now?

[dakrone]

I'm planning on adding a unit test for this, I wanted to get it up for discussion in case I missed something in the implementation. Hopefully a test will make it clearer

…

On Jan 20, 2018 9:23 AM, "Simon Willnauer" ***@***.***> wrote: @dakrone <https://github.com/dakrone> it's unclear to me what you are fixing here. is it possible to have a simple unittest that fails previously and passes now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28317 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABKdBQSN5aknqJzb6p_zqK3yncUNp-bks5tMhLzgaJpZM4RlL-m> .

[s1monw]

++ @dakrone

[jpountz]

This should be backported to 6.2, it's confusing to have a fix in the 6.1 and 6.3 series but not 6.2?

[s1monw]

absolutely this should go into 6.2

[s1monw]

@dakrone I looked at it to make sure I understand what's going on. I suggest this patch based on your changes:

diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java
index 4ecb8b1442..fd91a8a760 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java
@@ -597,21 +597,7 @@ public class Setting<T> implements ToXContentObject {
 
                 @Override
                 public boolean hasChanged(Settings current, Settings previous) {
-                    return Stream.concat(matchStream(current), matchStream(previous)).distinct().anyMatch(aKey -> {
-                        String namespace = key.getNamespace(aKey);
-                        Setting<T> concreteSetting = getConcreteSetting(aKey);
-                        AbstractScopedSettings.SettingUpdater<T> updater =
-                            concreteSetting.newUpdater((v) -> {}, logger, (v) -> validator.accept(namespace, v));
-                        T value = updater.getValue(current, previous);
-                        if (updater.hasChanged(current, previous)) {
-                            // only the ones that have changed otherwise we might get too many updates
-                            // the hasChanged above checks only if there are any changes
-                            if ((omitDefaults && value.equals(concreteSetting.getDefault(current))) == false) {
-                                return true;
-                            }
-                        }
-                        return false;
-                    });
+                    return current.filter(k -> match(k)).equals(previous.filter(k -> match(k))) == false;
                 }
 
                 @Override
@@ -623,8 +609,14 @@ public class Setting<T> implements ToXContentObject {
                         Setting<T> concreteSetting = getConcreteSetting(aKey);
                         AbstractScopedSettings.SettingUpdater<T> updater =
                             concreteSetting.newUpdater((v) -> {}, logger, (v) -> validator.accept(namespace, v));
-                        T value = updater.getValue(current, previous);
-                        result.put(namespace, value);
+                        if (updater.hasChanged(current, previous)) {
+                            // only the ones that have changed otherwise we might get too many updates
+                            // the hasChanged above checks only if there are any changes
+                            T value = updater.getValue(current, previous);
+                            if ((omitDefaults && value.equals(concreteSetting.getDefault(current))) == false) {
+                                result.put(namespace, value);
+                            }
+                        }
                     });
                     return result;
                 }
diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
index 298a82a138..fba760bc60 100644
--- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
+++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
@@ -226,6 +226,27 @@ public class ScopedSettingsTests extends ESTestCase {
         assertEquals(1, intResults.size());
     }
 
+    public void testPrefixKeySetting() {
+        Setting.AffixSetting<String> setting = Setting.prefixKeySetting("foo.",
+            (k) ->  Setting.simpleString(k, Property.Dynamic, Property.NodeScope));
+        Setting<String> stringSetting = Setting.simpleString("test.bar", Property.Dynamic, Property.NodeScope);
+        AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY,new HashSet<>(Arrays.asList(setting, stringSetting)));
+        Map<String, String> consumerMap = new HashMap<>();
+        service.addAffixMapUpdateConsumer(setting, (m) -> {
+            consumerMap.clear();
+            consumerMap.putAll(m);
+        }, (s, k) -> {}, false);
+        service.applySettings(Settings.builder().put("foo.bar", "1").put("foo.baz", "2").build());
+        assertEquals(2, consumerMap.size());
+        assertEquals("1", consumerMap.get("bar"));
+        assertEquals("2", consumerMap.get("baz"));
+        service.applySettings(Settings.builder().put("foo.bar", "1").put("foo.baz", "2").put("test.bar", "foo").build());
+
+        assertEquals(2, consumerMap.size());
+        assertEquals("1", consumerMap.get("bar"));
+        assertEquals("2", consumerMap.get("baz"));
+    }
+
     public void testAddConsumerAffixMap() {
         Setting.AffixSetting<Integer> intSetting = Setting.affixKeySetting("foo.", "bar",
             (k) ->  Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope));
@@ -261,6 +282,21 @@ public class ScopedSettingsTests extends ESTestCase {
         assertEquals(2, listResults.size());
         assertEquals(2, intResults.size());
 
+        service.applySettings(Settings.builder()
+            .put("foo.test.bar", 2)
+            .put("foo.test_1.bar", 7)
+            .putList("foo.test_list.list", "16", "17")
+            .putList("foo.test_list_1.list", "18", "19", "20")
+            .build());
+
+        assertEquals(2, intResults.get("test").intValue());
+        assertEquals(7, intResults.get("test_1").intValue());
+        assertEquals(Arrays.asList(16, 17), listResults.get("test_list"));
+        assertEquals(Arrays.asList(18, 19, 20), listResults.get("test_list_1"));
+        assertEquals(2, listResults.size());
+        assertEquals(2, intResults.size());
+
+
         listResults.clear();
         intResults.clear();
 
@@ -270,19 +306,18 @@ public class ScopedSettingsTests extends ESTestCase {
             .putList("foo.test_list.list", "16", "17")
             .putNull("foo.test_list_1.list")
             .build());
-        // TODO: should this assertion be kept?
-        // assertNull("test wasn't changed", intResults.get("test"));
+        assertNull("test wasn't changed", intResults.get("test"));
         assertEquals(8, intResults.get("test_1").intValue());
-        // assertNull("test_list wasn't changed", listResults.get("test_list"));
+        assertNull("test_list wasn't changed", listResults.get("test_list"));
         if (omitDefaults) {
             assertNull(listResults.get("test_list_1"));
             assertFalse(listResults.containsKey("test_list_1"));
             assertEquals(0, listResults.size());
-            assertEquals(2, intResults.size());
+            assertEquals(1, intResults.size());
         } else {
             assertEquals(Arrays.asList(1), listResults.get("test_list_1")); // reset to default
-            assertEquals(2, listResults.size());
-            assertEquals(2, intResults.size());
+            assertEquals(1, listResults.size());
+            assertEquals(1, intResults.size());
         }
 
     }

[dakrone]

@s1monw whoops I didn't see that before I started working on mine, I added a unit test that demonstrates this

[dakrone]

I will apply your comments, thanks Simon

[dakrone]

@s1monw thanks for the suggested patch, can you take another look please?

[s1monw]

LGTM

[jhalterman]

Do you think we should test the patch for persistent settings as well, or a mix of the two? We observed that issue effects persistent and transient settings.

[dakrone]

@jhalterman the unit test for this is outside the scope of persistent or transient (the fix is to the settings themselves, it actually doesn't matter whether the settings are transient or persistent), so I don't think a test for that is necessary.