From 5a03eb91e664e64041315c597f320b60b556d9a7 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 14 Oct 2016 23:55:15 -0400 Subject: [PATCH] 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. Relates #20951 --- .../metadata/MetaDataIndexUpgradeService.java | 9 +++- .../settings/AbstractScopedSettings.java | 32 ++++++++----- .../org/elasticsearch/gateway/Gateway.java | 28 +++++++++-- .../index/IndexSettingsTests.java | 48 ++++++++++++++----- 4 files changed, 88 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java index 27d28417b75..19bf924910e 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java @@ -19,6 +19,8 @@ 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; @@ -26,8 +28,8 @@ 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 @@ public class MetaDataIndexUpgradeService extends AbstractComponent { 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 { diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 9a392860096..e72f274fd62 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -498,11 +498,21 @@ public abstract class AbstractScopedSettings extends AbstractComponent { } /** - * 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> unknownConsumer, + final BiConsumer, IllegalArgumentException> invalidConsumer) { Settings.Builder builder = Settings.builder(); boolean changed = false; for (Map.Entry entry : settings.getAsMap().entrySet()) { @@ -516,10 +526,10 @@ public abstract class AbstractScopedSettings extends AbstractComponent { 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()); @@ -527,12 +537,10 @@ public abstract class AbstractScopedSettings extends AbstractComponent { } } 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()); diff --git a/core/src/main/java/org/elasticsearch/gateway/Gateway.java b/core/src/main/java/org/elasticsearch/gateway/Gateway.java index c0ac2bb56e0..3a6bfa7aec1 100644 --- a/core/src/main/java/org/elasticsearch/gateway/Gateway.java +++ b/core/src/main/java/org/elasticsearch/gateway/Gateway.java @@ -21,7 +21,6 @@ package org.elasticsearch.gateway; 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.index.Index; 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 class Gateway extends AbstractComponent implements ClusterStateListener { } } 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 e) { + logger.warn("ignoring unknown {} setting: [{}] with value [{}]; archiving", settingType, e.getKey(), e.getValue()); + } + + private void logInvalidSetting(String settingType, Map.Entry 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 diff --git a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java b/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java index 3909354c989..97a6c6abf70 100644 --- a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java +++ b/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.settings.Settings; 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 class IndexSettingsTests extends ESTestCase { 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")); } + }