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

Allow affix settings to specify dependencies #27161

Merged
merged 12 commits into from
Nov 13, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

"must be consistent on their own?"

Copy link
Member

Choose a reason for hiding this comment

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

"we can't allow dependencies between the two"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if you have setting A to be transient and then it disappears on a full cluster restart?

Copy link
Member

Choose a reason for hiding this comment

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

sure I agree, I was just suggesting to rephrase the comment :)

// 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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand reg.? It took me a few seconds to figure out what it meant (regards) and I do not want to have to do it every time I read this comment?

indexTemplateService.putTemplate(new MetaDataIndexTemplateService.PutRequest(cause, request.name())
.patterns(request.patterns())
.order(request.order())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
}

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 dependencies
} catch (IllegalArgumentException iae) {
validationErrors.add(iae.getMessage());
for (Throwable t : iae.getSuppressed()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

question: private settings are internal only, hence we want to get them out of the validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(finalSettings));
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if this second step wasn't enough, compared to the first validation step that doesn't look at dependencies. Why have the two steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one happens before we pass on to a clusterstate update task we should validate as soon as we can. but we have to do it here again

}
}
}
Expand All @@ -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));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,33 +264,28 @@ 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) {
Setting setting = get(key);
void validate(String key, Settings settings, boolean validateDependencies) {
Setting setting = getRaw(key);
if (setting == null) {
LevensteinDistance ld = new LevensteinDistance();
List<Tuple<Float, String>> scoredKeys = new ArrayList<>();
Expand All @@ -315,6 +310,20 @@ public final void validate(String key, Settings settings) {
"settings";
}
throw new IllegalArgumentException(msg);
} else {
Set<String> settingsDependencies = setting.getSettingsDependencies(key);
if (setting.hasComplexMatcher()) {
setting = setting.getConcreteSetting(key);
}
if (validateDependencies && settingsDependencies.isEmpty() == false) {
Set<String> settingKeys = settings.keySet();
for (String requiredSetting : settingsDependencies) {
if (settingKeys.contains(requiredSetting) == false) {
throw new IllegalArgumentException("Missing required setting ["
+ requiredSetting + "] for setting [" + setting.getKey() + "]");
}
}
}
}
setting.get(settings);
}
Expand Down Expand Up @@ -375,15 +384,27 @@ default Runnable updater(Settings current, Settings previous) {
/**
* Returns the {@link Setting} for the given key or <code>null</code> if the setting can not be found.
*/
public Setting<?> get(String key) {
public final Setting<?> get(String key) {
Setting<?> raw = getRaw(key);
if (raw == null) {
return null;
} if (raw.hasComplexMatcher()) {
return raw.getConcreteSetting(key);
} else {
return raw;
}
}

private Setting<?> getRaw(String key) {
Setting<?> setting = keySettings.get(key);
if (setting != null) {
return setting;
}
for (Map.Entry<String, Setting<?>> entry : complexMatchers.entrySet()) {
if (entry.getValue().match(key)) {
assert assertMatcher(key, 1);
return entry.getValue().getConcreteSetting(key);
assert entry.getValue().hasComplexMatcher();
return entry.getValue();
}
}
return null;
Expand Down Expand Up @@ -513,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;
Expand Down Expand Up @@ -654,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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,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:
Expand Down
41 changes: 35 additions & 6 deletions core/src/main/java/org/elasticsearch/common/settings/Setting.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -125,7 +126,7 @@ public enum Property {
private static final EnumSet<Property> EMPTY_PROPERTIES = EnumSet.noneOf(Property.class);

private Setting(Key key, @Nullable Setting<T> fallbackSetting, Function<Settings, String> defaultValue, Function<String, T> parser,
Validator<T> validator, Property... properties) {
Validator<T> validator, Property... properties) {
assert this instanceof SecureSetting || this.isGroupSetting() || parser.apply(defaultValue.apply(Settings.EMPTY)) != null
: "parser returned null";
this.key = key;
Expand Down Expand Up @@ -456,6 +457,14 @@ public Setting<T> getConcreteSetting(String key) {
return this;
}

/**
* Returns a set of settings that are required at validation time. Unless all of the dependencies are present in the settings
* object validation of setting must fail.
*/
public Set<String> getSettingsDependencies(String key) {
return Collections.emptySet();
}

/**
* Build a new updater with a noop validator.
*/
Expand Down Expand Up @@ -518,11 +527,13 @@ public String toString() {
public static class AffixSetting<T> extends Setting<T> {
private final AffixKey key;
private final Function<String, Setting<T>> delegateFactory;
private final Set<AffixSetting> dependencies;

public AffixSetting(AffixKey key, Setting<T> delegate, Function<String, Setting<T>> delegateFactory) {
public AffixSetting(AffixKey key, Setting<T> delegate, Function<String, Setting<T>> delegateFactory, AffixSetting... dependencies) {
super(key, delegate.defaultValue, delegate.parser, delegate.properties.toArray(new Property[0]));
this.key = key;
this.delegateFactory = delegateFactory;
this.dependencies = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(dependencies)));
}

boolean isGroupSetting() {
Expand All @@ -533,6 +544,15 @@ private Stream<String> matchStream(Settings settings) {
return settings.keySet().stream().filter((key) -> match(key)).map(settingKey -> key.getConcreteString(settingKey));
}

public Set<String> getSettingsDependencies(String settingsKey) {
if (dependencies.isEmpty()) {
return Collections.emptySet();
} else {
String namespace = key.getNamespace(settingsKey);
return dependencies.stream().map(s -> s.key.toConcreteKey(namespace).key).collect(Collectors.toSet());
}
}

AbstractScopedSettings.SettingUpdater<Map<AbstractScopedSettings.SettingUpdater<T>, T>> newAffixUpdater(
BiConsumer<String, T> consumer, Logger logger, BiConsumer<String, T> validator) {
return new AbstractScopedSettings.SettingUpdater<Map<AbstractScopedSettings.SettingUpdater<T>, T>>() {
Expand Down Expand Up @@ -658,6 +678,13 @@ public Stream<Setting<T>> getAllConcreteSettings(Settings settings) {
return matchStream(settings).distinct().map(this::getConcreteSetting);
}

/**
* Returns distinct namespaces for the given settings
*/
public Set<String> getNamespaces(Settings settings) {
return settings.keySet().stream().filter(this::match).map(key::getNamespace).collect(Collectors.toSet());
}

/**
* Returns a map of all namespaces to it's values give the provided settings
*/
Expand Down Expand Up @@ -1173,13 +1200,15 @@ public static <T> AffixSetting<T> prefixKeySetting(String prefix, Function<Strin
* storage.${backend}.enable=[true|false] can easily be added with this setting. Yet, affix key settings don't support updaters
* out of the box unless {@link #getConcreteSetting(String)} is used to pull the updater.
*/
public static <T> AffixSetting<T> affixKeySetting(String prefix, String suffix, Function<String, Setting<T>> delegateFactory) {
return affixKeySetting(new AffixKey(prefix, suffix), delegateFactory);
public static <T> AffixSetting<T> affixKeySetting(String prefix, String suffix, Function<String, Setting<T>> delegateFactory,
AffixSetting... dependencies) {
return affixKeySetting(new AffixKey(prefix, suffix), delegateFactory, dependencies);
}

private static <T> AffixSetting<T> affixKeySetting(AffixKey key, Function<String, Setting<T>> delegateFactory) {
private static <T> AffixSetting<T> affixKeySetting(AffixKey key, Function<String, Setting<T>> delegateFactory,
AffixSetting... dependencies) {
Setting<T> delegate = delegateFactory.apply("_na_");
return new AffixSetting<>(key, delegate, delegateFactory);
return new AffixSetting<>(key, delegate, delegateFactory, dependencies);
};


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Loading