Add precise logging on unknown or invalid settings

Today when logging an unknown or invalid setting, the log message does
not contain the source. This means that if we are archiving such a
setting, we do not specify where the setting is from (an index, and
which index, or a persistent or transient cluster setting). This commit
provides such logging for the end user can better understand the
consequences of the unknown or invalid setting.

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java

@@ -19,15 +19,17 @@
 package org.elasticsearch.cluster.metadata;
 
 import com.carrotsearch.hppc.cursors.ObjectCursor;
+import org.apache.logging.log4j.message.ParameterizedMessage;
+import org.apache.logging.log4j.util.Supplier;
 import org.apache.lucene.analysis.Analyzer;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.IndexScopedSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexSettings;
-import org.elasticsearch.index.analysis.IndexAnalyzers;
 import org.elasticsearch.index.analysis.AnalyzerScope;
+import org.elasticsearch.index.analysis.IndexAnalyzers;
 import org.elasticsearch.index.analysis.NamedAnalyzer;
 import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.similarity.SimilarityService;
@@ -161,7 +163,10 @@ private IndexMetaData markAsUpgraded(IndexMetaData indexMetaData) {
 
     IndexMetaData archiveBrokenIndexSettings(IndexMetaData indexMetaData) {
         final Settings settings = indexMetaData.getSettings();
-        final Settings upgrade = indexScopedSettings.archiveUnknownOrBrokenSettings(settings);
+        final Settings upgrade = indexScopedSettings.archiveUnknownOrInvalidSettings(
+            settings,
+            e -> logger.warn("{} ignoring unknown index setting: [{}] with value [{}]; archiving", indexMetaData.getIndex(), e.getKey(), e.getValue()),
+            (e, ex) -> logger.warn((Supplier<?>) () -> new ParameterizedMessage("{} ignoring invalid index setting: [{}] with value [{}]; archiving", indexMetaData.getIndex(), e.getKey(), e.getValue()), ex));
         if (upgrade != settings) {
             return IndexMetaData.builder(indexMetaData).settings(upgrade).build();
         } else {

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

@@ -498,11 +498,21 @@ private static Setting<?> findOverlappingSetting(Setting<?> newSetting, Map<Stri
     }
 
     /**
-     * Archives broken or unknown settings. Any setting that is not recognized or fails
-     * validation will be archived. This means the setting is prefixed with {@value ARCHIVED_SETTINGS_PREFIX}
-     * and remains in the settings object. This can be used to detect broken settings via APIs.
+     * Archives invalid or unknown settings. Any setting that is not recognized or fails validation
+     * will be archived. This means the setting is prefixed with {@value ARCHIVED_SETTINGS_PREFIX}
+     * and remains in the settings object. This can be used to detect invalid settings via APIs.
+     *
+     * @param settings        the {@link Settings} instance to scan for unknown or invalid settings
+     * @param unknownConsumer callback on unknown settings (consumer receives unknown key and its
+     *                        associated value)
+     * @param invalidConsumer callback on invalid settings (consumer receives invalid key, its
+     *                        associated value and an exception)
+     * @return a {@link Settings} instance with the unknown or invalid settings archived
      */
-    public Settings archiveUnknownOrBrokenSettings(Settings settings) {
+    public Settings archiveUnknownOrInvalidSettings(
+        final Settings settings,
+        final Consumer<Map.Entry<String, String>> unknownConsumer,
+        final BiConsumer<Map.Entry<String, String>, IllegalArgumentException> invalidConsumer) {
         Settings.Builder builder = Settings.builder();
         boolean changed = false;
         for (Map.Entry<String, String> entry : settings.getAsMap().entrySet()) {
@@ -516,23 +526,21 @@ public Settings archiveUnknownOrBrokenSettings(Settings settings) {
                         builder.put(entry.getKey(), entry.getValue());
                     } else {
                         changed = true;
-                        logger.warn("found unknown setting: {} value: {} - archiving", entry.getKey(), entry.getValue());
+                        unknownConsumer.accept(entry);
                         /*
-                         * We put them back in here such that tools can check from the outside if there are any indices with broken
-                         * settings. The setting can remain there but we want users to be aware that some of their setting are broken and
+                         * We put them back in here such that tools can check from the outside if there are any indices with invalid
+                         * settings. The setting can remain there but we want users to be aware that some of their setting are invalid and
                          * they can research why and what they need to do to replace them.
                          */
                         builder.put(ARCHIVED_SETTINGS_PREFIX + entry.getKey(), entry.getValue());
                     }
                 }
             } catch (IllegalArgumentException ex) {
                 changed = true;
-                logger.warn(
-                    (Supplier<?>) () -> new ParameterizedMessage(
-                        "found invalid setting: {} value: {} - archiving", entry.getKey(), entry.getValue()), ex);
+                invalidConsumer.accept(entry, ex);
                 /*
-                 * We put them back in here such that tools can check from the outside if there are any indices with broken settings. The
-                 * setting can remain there but we want users to be aware that some of their setting are broken and they can research why
+                 * We put them back in here such that tools can check from the outside if there are any indices with invalid settings. The
+                 * setting can remain there but we want users to be aware that some of their setting are invalid and they can research why
                  * and what they need to do to replace them.
                  */
                 builder.put(ARCHIVED_SETTINGS_PREFIX + entry.getKey(), entry.getValue());

core/src/main/java/org/elasticsearch/gateway/Gateway.java

@@ -21,7 +21,6 @@
 
 import com.carrotsearch.hppc.ObjectFloatHashMap;
 import com.carrotsearch.hppc.cursors.ObjectCursor;
-
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.elasticsearch.action.FailedNodeException;
 import org.elasticsearch.cluster.ClusterChangedEvent;
@@ -38,6 +37,7 @@
 import org.elasticsearch.indices.IndicesService;
 
 import java.util.Arrays;
+import java.util.Map;
 import java.util.function.Supplier;
 
 public class Gateway extends AbstractComponent implements ClusterStateListener {
@@ -146,13 +146,35 @@ public void performStateRecovery(final GatewayStateRecoveredListener listener) t
             }
         }
         final ClusterSettings clusterSettings = clusterService.getClusterSettings();
-        metaDataBuilder.persistentSettings(clusterSettings.archiveUnknownOrBrokenSettings(metaDataBuilder.persistentSettings()));
-        metaDataBuilder.transientSettings(clusterSettings.archiveUnknownOrBrokenSettings(metaDataBuilder.transientSettings()));
+        metaDataBuilder.persistentSettings(
+            clusterSettings.archiveUnknownOrInvalidSettings(
+                metaDataBuilder.persistentSettings(),
+                e -> logUnknownSetting("persistent", e),
+                (e, ex) -> logInvalidSetting("persistent", e, ex)));
+        metaDataBuilder.transientSettings(
+            clusterSettings.archiveUnknownOrInvalidSettings(
+                metaDataBuilder.transientSettings(),
+                e -> logUnknownSetting("transient", e),
+                (e, ex) -> logInvalidSetting("transient", e, ex)));
         ClusterState.Builder builder = ClusterState.builder(clusterService.getClusterName());
         builder.metaData(metaDataBuilder);
         listener.onSuccess(builder.build());
     }
 
+    private void logUnknownSetting(String settingType, Map.Entry<String, String> e) {
+        logger.warn("ignoring unknown {} setting: [{}] with value [{}]; archiving", settingType, e.getKey(), e.getValue());
+    }
+
+    private void logInvalidSetting(String settingType, Map.Entry<String, String> e, IllegalArgumentException ex) {
+        logger.warn(
+            (org.apache.logging.log4j.util.Supplier<?>)
+                () -> new ParameterizedMessage("ignoring invalid {} setting: [{}] with value [{}]; archiving",
+                    settingType,
+                    e.getKey(),
+                    e.getValue()),
+            ex);
+    }
+
     @Override
     public void clusterChanged(final ClusterChangedEvent event) {
         // order is important, first metaState, and then shardsState

core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java

@@ -16,11 +16,11 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+
 package org.elasticsearch.index;
 
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
-import org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeService;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.settings.IndexScopedSettings;
 import org.elasticsearch.common.settings.Setting;
@@ -29,18 +29,20 @@
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.index.translog.Translog;
-import org.elasticsearch.indices.mapper.MapperRegistry;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.VersionUtils;
 
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
 
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.core.StringContains.containsString;
+import static org.hamcrest.object.HasToString.hasToString;
+
 public class IndexSettingsTests extends ESTestCase {
 
     public void testRunListener() {
@@ -348,26 +350,48 @@ public void testTranslogFlushSizeThreshold() {
         assertEquals(actualNewTranslogFlushThresholdSize, settings.getFlushThresholdSize());
     }
 
-
     public void testArchiveBrokenIndexSettings() {
-        Settings settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(Settings.EMPTY);
+        Settings settings =
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings(
+                Settings.EMPTY,
+                e -> { assert false : "should not have been invoked, no unknown settings"; },
+                (e, ex) -> { assert false : "should not have been invoked, no invalid settings"; });
         assertSame(settings, Settings.EMPTY);
-        settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(Settings.builder()
-            .put("index.refresh_interval", "-200").build());
+        settings =
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings(
+                Settings.builder().put("index.refresh_interval", "-200").build(),
+                e -> { assert false : "should not have been invoked, no invalid settings"; },
+                (e, ex) -> {
+                    assertThat(e.getKey(), equalTo("index.refresh_interval"));
+                    assertThat(e.getValue(), equalTo("-200"));
+                    assertThat(ex, hasToString(containsString("failed to parse setting [index.refresh_interval] with value [-200]")));
+                });
         assertEquals("-200", settings.get("archived.index.refresh_interval"));
         assertNull(settings.get("index.refresh_interval"));
 
         Settings prevSettings = settings; // no double archive
-        settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(prevSettings);
+        settings =
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings(
+                prevSettings,
+                e -> { assert false : "should not have been invoked, no unknown settings"; },
+                (e, ex) -> { assert false : "should not have been invoked, no invalid settings"; });
         assertSame(prevSettings, settings);
 
-        settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(Settings.builder()
-            .put("index.version.created", Version.CURRENT.id) // private setting
-            .put("index.unknown", "foo")
-            .put("index.refresh_interval", "2s").build());
+        settings =
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings(
+                Settings.builder()
+                    .put("index.version.created", Version.CURRENT.id) // private setting
+                    .put("index.unknown", "foo")
+                    .put("index.refresh_interval", "2s").build(),
+                e -> {
+                    assertThat(e.getKey(), equalTo("index.unknown"));
+                    assertThat(e.getValue(), equalTo("foo"));
+                },
+                (e, ex) -> { assert false : "should not have been invoked, no invalid settings"; });
 
         assertEquals("foo", settings.get("archived.index.unknown"));
         assertEquals(Integer.toString(Version.CURRENT.id), settings.get("index.version.created"));
         assertEquals("2s", settings.get("index.refresh_interval"));
     }
+
 }