Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parameter to make sure that log of updating IndexSetting be more detailed #49969

Merged
merged 9 commits into from
Jan 3, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@

package org.elasticsearch.common.settings;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.lucene.search.spell.LevenshteinDistance;
import org.apache.lucene.util.CollectionUtil;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.regex.Regex;

import java.util.ArrayList;
Expand Down Expand Up @@ -55,7 +55,7 @@ public abstract class AbstractScopedSettings {
private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$");
private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+[*](?:[.][-\\w]+)+$");

protected final Logger logger = LogManager.getLogger(this.getClass());
protected final Logger logger;

private final Settings settings;
private final List<SettingUpdater<?>> settingUpdaters = new CopyOnWriteArrayList<>();
Expand All @@ -69,7 +69,9 @@ protected AbstractScopedSettings(
final Settings settings,
final Set<Setting<?>> settingsSet,
final Set<SettingUpgrader<?>> settingUpgraders,
final Setting.Property scope) {
final Setting.Property scope,
final String prefix) {
this.logger = Loggers.getLogger(this.getClass(), prefix);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to allow subclasses to provide their own logger rather than just a String, because I don't think we should be changing the behaviour for the logging of cluster-level settings.

this.settings = settings;
this.lastSettingsApplied = Settings.EMPTY;

Expand Down Expand Up @@ -110,7 +112,8 @@ protected void validateSettingKey(Setting<?> setting) {
}
}

protected AbstractScopedSettings(Settings nodeSettings, Settings scopeSettings, AbstractScopedSettings other) {
protected AbstractScopedSettings(Settings nodeSettings, Settings scopeSettings, AbstractScopedSettings other, String prefix) {
this.logger = Loggers.getLogger(this.getClass(), prefix);
this.settings = nodeSettings;
this.lastSettingsApplied = scopeSettings;
this.scope = other.scope;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public ClusterSettings(final Settings nodeSettings, final Set<Setting<?>> settin
}

public ClusterSettings(final Settings nodeSettings, final Set<Setting<?>> settingsSet, final Set<SettingUpgrader<?>> settingUpgraders) {
super(nodeSettings, settingsSet, settingUpgraders, Property.NodeScope);
super(nodeSettings, settingsSet, settingUpgraders, Property.NodeScope, "cluster");
addSettingsUpdater(new LoggingSettingUpdater(nodeSettings));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,15 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
public static final IndexScopedSettings DEFAULT_SCOPED_SETTINGS = new IndexScopedSettings(Settings.EMPTY, BUILT_IN_INDEX_SETTINGS);

public IndexScopedSettings(Settings settings, Set<Setting<?>> settingsSet) {
super(settings, settingsSet, Collections.emptySet(), Property.IndexScope);
super(settings, settingsSet, Collections.emptySet(), Property.IndexScope, "indexScopedSettings");
}

public IndexScopedSettings(Settings settings, Set<Setting<?>> settingsSet, String prefix) {
super(settings, settingsSet, Collections.emptySet(), Property.IndexScope, prefix);
}

private IndexScopedSettings(Settings settings, IndexScopedSettings other, IndexMetaData metaData) {
super(settings, metaData.getSettings(), other);
super(settings, metaData.getSettings(), other, metaData.getIndex().getName());
}

public IndexScopedSettings copy(Settings settings, IndexMetaData metaData) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,21 @@
*/
package org.elasticsearch.common.settings;

import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.appender.AbstractAppender;
import org.apache.logging.log4j.core.filter.RegexFilter;
import org.apache.logging.log4j.core.impl.MementoMessage;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.settings.AbstractScopedSettings.SettingUpdater;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.monitor.jvm.JvmInfo;
import org.elasticsearch.test.ESTestCase;

Expand All @@ -40,6 +49,7 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.elasticsearch.index.IndexSettingsTests.newIndexMeta;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
Expand All @@ -49,6 +59,27 @@

public class SettingTests extends ESTestCase {

public static class MockAppender extends AbstractAppender {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is now unused

private LogEvent lastEvent;

public MockAppender(final String name) throws IllegalAccessException {
super(name, RegexFilter.createFilter(".*(\n.*)*", new String[0], false, null, null), null);
}

@Override
public void append(LogEvent event) {
lastEvent = event.toImmutable();
}

public MementoMessage lastMementoMessage() {
return (MementoMessage) lastEvent.getMessage();
}

public String getLoggerMaker() {
return lastEvent.getMarker().getName();
}
}

public void testGet() {
Setting<Boolean> booleanSetting = Setting.boolSetting("foo.bar", false, Property.Dynamic, Property.NodeScope);
assertFalse(booleanSetting.get(Settings.EMPTY));
Expand Down Expand Up @@ -1092,4 +1123,22 @@ public void testNonSecureSettingInKeystore() {
assertThat(e.getMessage(), containsString("must be stored inside elasticsearch.yml"));
}

public void testLogSettingUpdate() throws Exception {
IndexMetaData metaData = newIndexMeta("index1", Settings.builder()
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
.put(IndexSettings.INDEX_WARMER_ENABLED_SETTING.getKey(), false)
.build());
IndexSettings settings = new IndexSettings(metaData, Settings.EMPTY);
Logger logger = settings.getScopedSettings().logger;

MockAppender appender = new MockAppender("trace_appender");
kkewwei marked this conversation as resolved.
Show resolved Hide resolved
appender.start();
Loggers.addAppender(logger, appender);

settings.updateIndexMetaData(newIndexMeta("index1", Settings.builder().put(IndexSettings.INDEX_WARMER_ENABLED_SETTING.getKey(),
"true").build()));

assertThat(appender.lastMementoMessage().getFormattedMessage(), equalTo("updating [index.warmer.enabled] from [false] to [true]"));
assertThat(appender.getLoggerMaker(), equalTo("[index1]"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static IndexSettings newIndexSettings(Index index, Settings indexSetting,
if (setting.length > 0) {
settingSet.addAll(Arrays.asList(setting));
}
return new IndexSettings(metaData, nodeSettings, new IndexScopedSettings(Settings.EMPTY, settingSet));
return new IndexSettings(metaData, nodeSettings, new IndexScopedSettings(Settings.EMPTY, settingSet, index.getName()));
}

public static IndexSettings newIndexSettings(Index index, Settings settings, IndexScopedSettings indexScopedSettings) {
Expand All @@ -85,6 +85,7 @@ public static IndexSettings newIndexSettings(final IndexMetaData indexMetaData,
if (setting.length > 0) {
settingSet.addAll(Arrays.asList(setting));
}
return new IndexSettings(indexMetaData, Settings.EMPTY, new IndexScopedSettings(Settings.EMPTY, settingSet));
return new IndexSettings(indexMetaData, Settings.EMPTY,
new IndexScopedSettings(Settings.EMPTY, settingSet, indexMetaData.getIndex().getName()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ private IllegalArgumentException assertError(String realmType, String realmName,
private void validate(Settings settings) {
final Set<Setting<?>> settingsSet = new HashSet<>(InternalRealmsSettings.getSettings());
final AbstractScopedSettings validator = new AbstractScopedSettings(settings, settingsSet, Collections.emptySet(),
Setting.Property.NodeScope) {
Setting.Property.NodeScope, "realm") {
};
validator.validate(settings, false);
}
Expand Down