Skip to content

Commit

Permalink
Add precise logging on unknown or invalid settings
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jasontedor
jasontedor committed Oct 15, 2016
1 parent b2feefa commit 1135eb9
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 27 deletions.
7 changes: 6 additions & 1 deletion core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
32 changes: 20 additions & 12 deletions core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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());
Expand Down
27 changes: 25 additions & 2 deletions core/src/main/java/org/elasticsearch/gateway/Gateway.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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 {
Expand Down Expand Up @@ -146,13 +147,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
Expand Down
48 changes: 36 additions & 12 deletions core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down Expand Up @@ -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"));
}

}

0 comments on commit 1135eb9

Please sign in to comment.