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 notion of internal index settings #31286

Merged
merged 2 commits into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request
Settings.Builder settingsForOpenIndices = Settings.builder();
final Set<String> skippedSettings = new HashSet<>();

indexScopedSettings.validate(normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false /* don't validate wildcards */),
false); //don't validate dependencies here we check it below never allow to change the number of shards
indexScopedSettings.validate(
normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false), // don't validate wildcards
false, // don't validate dependencies here we check it below never allow to change the number of shards
true); // validate internal index settings
for (String key : normalizedSettings.keySet()) {
Setting setting = indexScopedSettings.get(key);
boolean isWildcard = setting == null && Regex.isSimpleMatchPattern(key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,18 @@ public final void validate(final Settings settings, final boolean validateDepend
validate(settings, validateDependencies, false, false);
}

/**
* Validates that all settings are registered and valid.
*
* @param settings the settings to validate
* @param validateDependencies true if dependent settings should be validated
* @param validateInternalIndex true if internal index settings should be validated
* @see Setting#getSettingsDependencies(String)
*/
public final void validate(final Settings settings, final boolean validateDependencies, final boolean validateInternalIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Think it might be better to either make validateInternalIndex more explicit and be validateInternalIndexSettings or remove "Index" and have it as validateInternal or validateInternalSettings. The reason is that at first glance this looks like its a flag for whether we validate settings in "internal indexes".

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think that this isn't clear enough from the provided Javadocs? validateInternalIndexSettings is such a long name and it would cause some lines to spill over the line length limit. I am happy to adjust if you feel strongly, but I want to double check before making the change because of the reformatting it will require.

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 its ok when you read the JavaDoc but it just seems a bit strange when first reading the variable name. I agree that validateInternalIndexSettings is too long. Are you against making it validateInternalSettings instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am hesitant on validateInternalSettings because we do not have a general notion of internal settings, only internal index settings for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thats true. In which case I'm ok with leaving this as is.

validate(settings, validateDependencies, false, false, validateInternalIndex);
}

/**
* Validates that all settings are registered and valid.
*
Expand All @@ -296,6 +308,25 @@ public final void validate(
final boolean validateDependencies,
final boolean ignorePrivateSettings,
final boolean ignoreArchivedSettings) {
validate(settings, validateDependencies, ignorePrivateSettings, ignoreArchivedSettings, false);
}

/**
* Validates that all settings are registered and valid.
*
* @param settings the settings
* @param validateDependencies true if dependent settings should be validated
* @param ignorePrivateSettings true if private settings should be ignored during validation
* @param ignoreArchivedSettings true if archived settings should be ignored during validation
* @param validateInternalIndex true if index internal settings should be validated
* @see Setting#getSettingsDependencies(String)
*/
public final void validate(
final Settings settings,
final boolean validateDependencies,
final boolean ignorePrivateSettings,
final boolean ignoreArchivedSettings,
final boolean validateInternalIndex) {
final List<RuntimeException> exceptions = new ArrayList<>();
for (final String key : settings.keySet()) { // settings iterate in deterministic fashion
if (isPrivateSetting(key) && ignorePrivateSettings) {
Expand All @@ -305,7 +336,7 @@ public final void validate(
continue;
}
try {
validate(key, settings, validateDependencies);
validate(key, settings, validateDependencies, validateInternalIndex);
} catch (final RuntimeException ex) {
exceptions.add(ex);
}
Expand All @@ -314,9 +345,27 @@ public final void validate(
}

/**
* Validates that the setting is valid
* Validates that the settings is valid.
*
* @param key the key of the setting to validate
* @param settings the settings
* @param validateDependencies true if dependent settings should be validated
* @throws IllegalArgumentException if the setting is invalid
*/
void validate(String key, Settings settings, boolean validateDependencies) {
void validate(final String key, final Settings settings, final boolean validateDependencies) {
validate(key, settings, validateDependencies, false);
}

/**
* Validates that the settings is valid.
*
* @param key the key of the setting to validate
* @param settings the settings
* @param validateDependencies true if dependent settings should be validated
* @param validateInternalIndex true if internal index settings should be validated
* @throws IllegalArgumentException if the setting is invalid
*/
void validate(final String key, final Settings settings, final boolean validateDependencies, final boolean validateInternalIndex) {
Setting setting = getRaw(key);
if (setting == null) {
LevensteinDistance ld = new LevensteinDistance();
Expand Down Expand Up @@ -356,6 +405,11 @@ void validate(String key, Settings settings, boolean validateDependencies) {
}
}
}
// the only time that validateInternalIndex should be true is if this call is coming via the update settings API
if (validateInternalIndex && setting.getProperties().contains(Setting.Property.InternalIndex)) {
throw new IllegalArgumentException(
"can not update internal setting [" + setting.getKey() + "]; this setting is managed via a dedicated API");
}
}
setting.get(settings);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,13 @@ public enum Property {
* Mark this setting as not copyable during an index resize (shrink or split). This property can only be applied to settings that
* also have {@link Property#IndexScope}.
*/
NotCopyableOnResize
NotCopyableOnResize,

/**
* Indicates an index-level setting that is managed internally. Such a setting can only be added to an index on index creation but
* can not be updated via the update API.
*/
InternalIndex
}

private final Key key;
Expand Down Expand Up @@ -152,14 +158,18 @@ private Setting(Key key, @Nullable Setting<T> fallbackSetting, Function<Settings
if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.Final)) {
throw new IllegalArgumentException("final setting [" + key + "] cannot be dynamic");
}
if (propertiesAsSet.contains(Property.NotCopyableOnResize) && propertiesAsSet.contains(Property.IndexScope) == false) {
throw new IllegalArgumentException(
"non-index-scoped setting [" + key + "] can not have property [" + Property.NotCopyableOnResize + "]");
}
checkPropertyRequiresIndexScope(propertiesAsSet, Property.NotCopyableOnResize);
checkPropertyRequiresIndexScope(propertiesAsSet, Property.InternalIndex);
this.properties = propertiesAsSet;
}
}

private void checkPropertyRequiresIndexScope(final EnumSet<Property> properties, final Property property) {
if (properties.contains(property) && properties.contains(Property.IndexScope) == false) {
throw new IllegalArgumentException("non-index-scoped setting [" + key + "] can not have property [" + property + "]");
}
}

/**
* Creates a new Setting instance
* @param key the settings key for this setting.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -876,4 +876,28 @@ public void testFinalSettingUpdateFail() {
Settings.builder().put(currentSettings), Settings.builder(), "node"));
assertThat(exc.getMessage(), containsString("final node setting [some.final.group.foo]"));
}

public void testInternalIndexSettingsFailsValidation() {
final Setting<String> indexInternalSetting = Setting.simpleString("index.internal", Property.InternalIndex, Property.IndexScope);
final IndexScopedSettings indexScopedSettings =
new IndexScopedSettings(Settings.EMPTY, Collections.singleton(indexInternalSetting));
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> {
final Settings settings = Settings.builder().put("index.internal", "internal").build();
indexScopedSettings.validate(settings, false, /* validateInternalIndex */ true);
});
final String message = "can not update internal setting [index.internal]; this setting is managed via a dedicated API";
assertThat(e, hasToString(containsString(message)));
}

public void testInternalIndexSettingsSkipValidation() {
final Setting<String> internalIndexSetting = Setting.simpleString("index.internal", Property.InternalIndex, Property.IndexScope);
final IndexScopedSettings indexScopedSettings =
new IndexScopedSettings(Settings.EMPTY, Collections.singleton(internalIndexSetting));
// nothing should happen, validation should not throw an exception
final Settings settings = Settings.builder().put("index.internal", "internal").build();
indexScopedSettings.validate(settings, false, /* validateInternalIndex */ false);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,13 @@ public void testRejectNonIndexScopedNotCopyableOnResizeSetting() {
assertThat(e, hasToString(containsString("non-index-scoped setting [foo.bar] can not have property [NotCopyableOnResize]")));
}

public void testRejectNonIndexScopedIndexInternalSetting() {
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> Setting.simpleString("foo.bar", Property.InternalIndex));
assertThat(e, hasToString(containsString("non-index-scoped setting [foo.bar] can not have property [InternalIndex]")));
}

public void testTimeValue() {
final TimeValue random = TimeValue.parseTimeValue(randomTimeValue(), "test");

Expand Down
Loading