-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Changes from 6 commits
c398ff0
ddd564c
9417ac6
278f416
9ef0b0f
9c0d9de
7ea1849
e8e25fb
f3cf5a8
a5d5135
da2878e
b73286e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you expand |
||
indexTemplateService.putTemplate(new MetaDataIndexTemplateService.PutRequest(cause, request.name()) | ||
.patterns(request.patterns()) | ||
.order(request.order()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 dependecies | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dependencies |
||
} catch (IllegalArgumentException iae) { | ||
validationErrors.add(iae.getMessage()); | ||
for (Throwable t : iae.getSuppressed()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah |
||
metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(finalSettings)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
} | ||
} | ||
|
@@ -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)); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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. | ||
*/ | ||
|
@@ -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() { | ||
|
@@ -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>>() { | ||
|
@@ -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((key) -> match(key)).map(key::getNamespace).collect(Collectors.toSet()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: match(key) could be replaced with a method reference |
||
} | ||
|
||
/** | ||
* Returns a map of all namespaces to it's values give the provided settings | ||
*/ | ||
|
@@ -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); | ||
}; | ||
|
||
|
||
|
There was a problem hiding this comment.
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?"
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)