apply feedback from @javanna

elastic · Nov 1, 2017 · 278f416 · 278f416
1 parent 9417ac6
commit 278f416
Show file tree

Hide file tree

Showing 11 changed files with 186 additions and 52 deletions.
diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java
@@ -54,15 +54,23 @@ synchronized ClusterState updateSettings(final ClusterState currentState, Settin
         transientSettings.put(currentState.metaData().transientSettings());
         changed |= clusterSettings.updateDynamicSettings(transientToApply, transientSettings, transientUpdates, "transient");
 
+
         Settings.Builder persistentSettings = Settings.builder();
         persistentSettings.put(currentState.metaData().persistentSettings());
         changed |= clusterSettings.updateDynamicSettings(persistentToApply, persistentSettings, persistentUpdates, "persistent");
 
         final ClusterState clusterState;
         if (changed) {
+            Settings transientFinalSettings = transientSettings.build();
+            Settings persistentFinalSettings = persistentSettings.build();
+            // both transient and persistent settings must be consistent by itself we can't allow dependencies to be
+            // in either of them otherwise a full cluster restart will break the settings validation
+            clusterSettings.validate(transientFinalSettings, true);
+            clusterSettings.validate(persistentFinalSettings, true);
+
             MetaData.Builder metaData = MetaData.builder(currentState.metaData())
-                    .persistentSettings(persistentSettings.build())
-                    .transientSettings(transientSettings.build());
+                    .persistentSettings(persistentFinalSettings)
+                    .transientSettings(transientFinalSettings);
 
             ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks());
             boolean updatedReadOnly = MetaData.SETTING_READ_ONLY_SETTING.get(metaData.persistentSettings())

diff --git a/.../org/elasticsearch/action/admin/indices/template/put/TransportPutIndexTemplateAction.java b/.../org/elasticsearch/action/admin/indices/template/put/TransportPutIndexTemplateAction.java
@@ -77,7 +77,7 @@ protected void masterOperation(final PutIndexTemplateRequest request, final Clus
         }
         final Settings.Builder templateSettingsBuilder = Settings.builder();
         templateSettingsBuilder.put(request.settings()).normalizePrefix(IndexMetaData.INDEX_SETTING_PREFIX);
-        indexScopedSettings.validate(templateSettingsBuilder);
+        indexScopedSettings.validate(templateSettingsBuilder.build(), true); // templates must be consistent with reg. to dependencies
         indexTemplateService.putTemplate(new MetaDataIndexTemplateService.PutRequest(cause, request.name())
                 .patterns(request.patterns())
                 .order(request.order())

diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java
@@ -116,7 +116,6 @@ public class MetaDataCreateIndexService extends AbstractComponent {
     private final IndexScopedSettings indexScopedSettings;
     private final ActiveShardsObserver activeShardsObserver;
     private final NamedXContentRegistry xContentRegistry;
-    private final ThreadPool threadPool;
 
     @Inject
     public MetaDataCreateIndexService(Settings settings, ClusterService clusterService,
@@ -132,7 +131,6 @@ public MetaDataCreateIndexService(Settings settings, ClusterService clusterServi
         this.env = env;
         this.indexScopedSettings = indexScopedSettings;
         this.activeShardsObserver = new ActiveShardsObserver(settings, clusterService, threadPool);
-        this.threadPool = threadPool;
         this.xContentRegistry = xContentRegistry;
     }
 
@@ -221,10 +219,9 @@ public void createIndex(final CreateIndexClusterStateUpdateRequest request,
     private void onlyCreateIndex(final CreateIndexClusterStateUpdateRequest request,
                                  final ActionListener<ClusterStateUpdateResponse> listener) {
         Settings.Builder updatedSettingsBuilder = Settings.builder();
-        updatedSettingsBuilder.put(request.settings()).normalizePrefix(IndexMetaData.INDEX_SETTING_PREFIX);
-        indexScopedSettings.validate(updatedSettingsBuilder);
-        request.settings(updatedSettingsBuilder.build());
-
+        Settings build = updatedSettingsBuilder.put(request.settings()).normalizePrefix(IndexMetaData.INDEX_SETTING_PREFIX).build();
+        indexScopedSettings.validate(build, true); // we do validate here - index setting must be consistent
+        request.settings(build);
         clusterService.submitStateUpdateTask("create-index [" + request.index() + "], cause [" + request.cause() + "]",
             new IndexCreationTask(logger, allocationService, request, listener, indicesService, aliasValidator, xContentRegistry, settings,
                 this::validate));
@@ -415,7 +412,6 @@ public ClusterState execute(ClusterState currentState) throws Exception {
                         tmpImdBuilder.primaryTerm(shardId, primaryTerm);
                     }
                 }
-
                 // Set up everything, now locally create the index to see that things are ok, and apply
                 final IndexMetaData tmpImd = tmpImdBuilder.build();
                 ActiveShardCount waitForActiveShards = request.waitForActiveShards();

diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java
@@ -276,7 +276,7 @@ private void validate(PutRequest request) {
         }
 
         try {
-            indexScopedSettings.validate(request.settings);
+            indexScopedSettings.validate(request.settings, true); // templates must be consistent with regards to dependecies
         } catch (IllegalArgumentException iae) {
             validationErrors.add(iae.getMessage());
             for (Throwable t : iae.getSuppressed()) {

diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java
@@ -54,6 +54,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Predicate;
 
 import static org.elasticsearch.action.support.ContextPreservingActionListener.wrapPreservingContext;
 
@@ -163,7 +164,7 @@ public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request
         Settings.Builder settingsForOpenIndices = Settings.builder();
         final Set<String> skippedSettings = new HashSet<>();
 
-        indexScopedSettings.validate(normalizedSettings);
+        indexScopedSettings.validate(normalizedSettings, false); // don't validate dependencies here we check it below
         // never allow to change the number of shards
         for (String key : normalizedSettings.keySet()) {
             Setting setting = indexScopedSettings.get(key);
@@ -240,7 +241,9 @@ public ClusterState execute(ClusterState currentState) {
                             if (preserveExisting) {
                                 indexSettings.put(indexMetaData.getSettings());
                             }
-                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(indexSettings));
+                            Settings finalSettings = indexSettings.build();
+                            indexScopedSettings.validate(finalSettings.filter(k -> indexScopedSettings.isPrivateSetting(k) == false), true);
+                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(finalSettings));
                         }
                     }
                 }
@@ -254,7 +257,9 @@ public ClusterState execute(ClusterState currentState) {
                             if (preserveExisting) {
                                 indexSettings.put(indexMetaData.getSettings());
                             }
-                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(indexSettings));
+                            Settings finalSettings = indexSettings.build();
+                            indexScopedSettings.validate(finalSettings.filter(k -> indexScopedSettings.isPrivateSetting(k) == false), true);
+                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(finalSettings));
                         }
                     }
                 }

diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
@@ -264,32 +264,27 @@ public synchronized <T> void addSettingsUpdateConsumer(Setting<T> setting, Consu
     }
 
     /**
-     * Validates that all settings in the builder are registered and valid
+     * Validates that all given settings are registered and valid
+     * @param settings the settings to validate
+     * @param validateDependencies if <code>true</code> settings dependencies are validated as well.
+     * @see Setting#getSettingsDependencies(String)
      */
-    public final void validate(Settings.Builder settingsBuilder) {
-        validate(settingsBuilder.build());
-    }
-
-    /**
-     * * Validates that all given settings are registered and valid
-     */
-    public final void validate(Settings settings) {
+    public final void validate(Settings settings, boolean validateDependencies) {
         List<RuntimeException> exceptions = new ArrayList<>();
         for (String key : settings.keySet()) { // settings iterate in deterministic fashion
             try {
-                validate(key, settings);
+                validate(key, settings, validateDependencies);
             } catch (RuntimeException ex) {
                 exceptions.add(ex);
             }
         }
         ExceptionsHelper.rethrowAndSuppress(exceptions);
     }
 
-
     /**
      * Validates that the setting is valid
      */
-    public final void validate(String key, Settings settings) {
+    void validate(String key, Settings settings, boolean validateDependencies) {
         Setting setting = getRaw(key);
         if (setting == null) {
             LevensteinDistance ld = new LevensteinDistance();
@@ -315,12 +310,12 @@ public final void validate(String key, Settings settings) {
                     "settings";
             }
             throw new IllegalArgumentException(msg);
-        } else {
+        } else  {
             Set<String> settingsDependencies = setting.getSettingsDependencies(key);
             if (setting.hasComplexMatcher()) {
                 setting = setting.getConcreteSetting(key);
             }
-            if (settingsDependencies.isEmpty() == false) {
+            if (validateDependencies && settingsDependencies.isEmpty() == false) {
                 Set<String> settingKeys = settings.keySet();
                 for (String requiredSetting : settingsDependencies) {
                     if (settingKeys.contains(requiredSetting) == false) {
@@ -539,7 +534,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin
             } else if (get(key) == null) {
                 throw new IllegalArgumentException(type + " setting [" + key + "], not recognized");
             } else if (isNull == false && canUpdate.test(key)) {
-                validate(key, toApply);
+                validate(key, toApply, false); // we might not have a full picture here do to a dependency validation
                 settingsBuilder.copy(key, toApply);
                 updates.copy(key, toApply);
                 changed = true;
@@ -680,7 +675,7 @@ public String setValue(String value) {
      * representation. Otherwise <code>false</code>
      */
     // TODO this should be replaced by Setting.Property.HIDDEN or something like this.
-    protected boolean isPrivateSetting(String key) {
+    public boolean isPrivateSetting(String key) {
         return false;
     }
 }
diff --git a/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java
@@ -189,7 +189,7 @@ protected void validateSettingKey(Setting setting) {
     }
 
     @Override
-    protected boolean isPrivateSetting(String key) {
+    public boolean isPrivateSetting(String key) {
         switch (key) {
             case IndexMetaData.SETTING_CREATION_DATE:
             case IndexMetaData.SETTING_INDEX_UUID:

diff --git a/core/src/main/java/org/elasticsearch/common/settings/SettingsModule.java b/core/src/main/java/org/elasticsearch/common/settings/SettingsModule.java
@@ -132,7 +132,7 @@ public SettingsModule(Settings settings, List<Setting<?>> additionalSettings, Li
             }
         }
         // by now we are fully configured, lets check node level settings for unregistered index settings
-        clusterSettings.validate(settings);
+        clusterSettings.validate(settings, true);
         this.settingsFilter = new SettingsFilter(settings, settingsFilterPattern);
      }
 

diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
@@ -122,7 +122,7 @@ public void testResetSettingWithIPValidator() {
         Settings.Builder builder = Settings.builder();
         Settings updates = Settings.builder().putNull("index.routing.allocation.require._ip")
             .put("index.some.dyn.setting", 1).build();
-        settings.validate(updates);
+        settings.validate(updates, false);
         settings.updateDynamicSettings(updates,
             Settings.builder().put(currentSettings), builder, "node");
         currentSettings = builder.build();
@@ -169,13 +169,15 @@ public void testDependentSettings() {
         AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY,new HashSet<>(Arrays.asList(intSetting, stringSetting)));
 
         IllegalArgumentException iae = expectThrows(IllegalArgumentException.class,
-            () -> service.validate(Settings.builder().put("foo.test.bar", 7).build()));
+            () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true));
         assertEquals("Missing required setting [foo.test.name] for setting [foo.test.bar]", iae.getMessage());
 
-        service.applySettings(Settings.builder()
+        service.validate(Settings.builder()
             .put("foo.test.name", "test")
             .put("foo.test.bar", 7)
-            .build());
+            .build(), true);
+
+        service.validate(Settings.builder().put("foo.test.bar", 7).build(), false);
     }
 
     public void testAddConsumerAffix() {
@@ -603,7 +605,7 @@ public void testValidateWithSuggestion() {
             Settings.EMPTY,
             IndexScopedSettings.BUILT_IN_INDEX_SETTINGS);
         IllegalArgumentException iae = expectThrows(IllegalArgumentException.class,
-            () -> settings.validate(Settings.builder().put("index.numbe_of_replica", "1").build()));
+            () -> settings.validate(Settings.builder().put("index.numbe_of_replica", "1").build(), false));
         assertEquals(iae.getMessage(), "unknown setting [index.numbe_of_replica] did you mean [index.number_of_replicas]?");
     }
 
@@ -613,26 +615,23 @@ public void testValidate() {
             IndexScopedSettings.BUILT_IN_INDEX_SETTINGS);
         String unknownMsgSuffix = " please check that any required plugins are installed, or check the breaking changes documentation for" +
             " removed settings";
-        settings.validate(Settings.builder().put("index.store.type", "boom"));
-        settings.validate(Settings.builder().put("index.store.type", "boom").build());
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate(Settings.builder().put("index.store.type", "boom").put("i.am.not.a.setting", true)));
-        assertEquals("unknown setting [i.am.not.a.setting]" + unknownMsgSuffix, e.getMessage());
+        settings.validate(Settings.builder().put("index.store.type", "boom").build(), false);
 
-        e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate(Settings.builder().put("index.store.type", "boom").put("i.am.not.a.setting", true).build()));
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
+            settings.validate(Settings.builder().put("index.store.type", "boom").put("i.am.not.a.setting", true).build(), false));
         assertEquals("unknown setting [i.am.not.a.setting]" + unknownMsgSuffix, e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate(Settings.builder().put("index.store.type", "boom").put("index.number_of_replicas", true).build()));
+            settings.validate(Settings.builder().put("index.store.type", "boom").put("index.number_of_replicas", true).build(), false));
         assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate("index.number_of_replicas", Settings.builder().put("index.number_of_replicas", "true").build()));
+            settings.validate("index.number_of_replicas", Settings.builder().put("index.number_of_replicas", "true").build(), false));
         assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate("index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build()));
+            settings.validate("index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build(),
+                false));
         assertEquals("illegal value for [index.similarity.classic] cannot redefine built-in similarity", e.getMessage());
     }
 
@@ -642,12 +641,12 @@ public void testValidateSecureSettings() {
         Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
         final ClusterSettings clusterSettings = new ClusterSettings(settings, Collections.emptySet());
 
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> clusterSettings.validate(settings));
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> clusterSettings.validate(settings, false));
         assertThat(e.getMessage(), startsWith("unknown secure setting [some.secure.setting]"));
 
         ClusterSettings clusterSettings2 = new ClusterSettings(settings,
             Collections.singleton(SecureSetting.secureString("some.secure.setting", null)));
-        clusterSettings2.validate(settings);
+        clusterSettings2.validate(settings, false);
     }
 
     public void testDiffSecureSettings() {
@@ -740,7 +739,7 @@ public void testLoggingUpdates() {
             IllegalArgumentException ex =
                 expectThrows(
                     IllegalArgumentException.class,
-                    () -> settings.validate(Settings.builder().put("logger._root", "boom").build()));
+                    () -> settings.validate(Settings.builder().put("logger._root", "boom").build(), false));
             assertEquals("Unknown level constant [BOOM].", ex.getMessage());
             assertEquals(level, ESLoggerFactory.getRootLogger().getLevel());
             settings.applySettings(Settings.builder().put("logger._root", "TRACE").build());

diff --git a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java b/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java
@@ -498,7 +498,7 @@ public void testSingleTypeSetting() {
             assertTrue(index.isSingleType());
             expectThrows(IllegalArgumentException.class, () -> {
                 index.getScopedSettings()
-                    .validate(Settings.builder().put(IndexSettings.INDEX_MAPPING_SINGLE_TYPE_SETTING_KEY, randomBoolean()).build());
+                    .validate(Settings.builder().put(IndexSettings.INDEX_MAPPING_SINGLE_TYPE_SETTING_KEY, randomBoolean()).build(), false);
             });
         }
         {