Add validation of restored persistent settings #9051

Merged
merged 1 commit into from Jan 18, 2015

Projects

None yet

3 participants

@imotov
Member
imotov commented Dec 24, 2014

Closes #8830

@tlrx tlrx and 1 other commented on an outdated diff Jan 6, 2015
...h/snapshots/DedicatedClusterSnapshotRestoreTests.java
@@ -82,16 +84,33 @@
@Test
public void restorePersistentSettingsTest() throws Exception {
- logger.info("--> start node");
- internalCluster().startNode(settingsBuilder().put("gateway.type", "local"));
+ logger.info("--> start 2 nodes");
+ Settings nodeSettings = settingsBuilder()
+ .put("gateway.type", "local")
+ .put("discovery.type", "zen")
+ .put("discovery.zen.ping_timeout", "200ms")
+ .put("discovery.initial_state_timeout", "500ms")
+ .put("gateway.type", "local")
@tlrx
tlrx Jan 6, 2015 Member

Duplicate "gateway.type"

Note that gateway.local will be removed in #9128

@imotov
imotov Jan 9, 2015 Member

Good point. Removing.

@tlrx tlrx commented on the diff Jan 6, 2015
.../java/org/elasticsearch/snapshots/RestoreService.java
@@ -283,7 +290,24 @@ private void validateExistingIndex(IndexMetaData currentIndexMetaData, IndexMeta
private void restoreGlobalStateIfRequested(MetaData.Builder mdBuilder) {
if (request.includeGlobalState()) {
if (metaData.persistentSettings() != null) {
- mdBuilder.persistentSettings(metaData.persistentSettings());
+ boolean changed = false;
+ ImmutableSettings.Builder persistentSettings = ImmutableSettings.settingsBuilder().put();
+ for (Map.Entry<String, String> entry : metaData.persistentSettings().getAsMap().entrySet()) {
+ if (dynamicSettings.isDynamicOrLoggingSetting(entry.getKey())) {
+ String error = dynamicSettings.validateDynamicSetting(entry.getKey(), entry.getValue());
+ if (error == null) {
+ persistentSettings.put(entry.getKey(), entry.getValue());
+ changed = true;
+ } else {
+ logger.warn("ignoring persistent setting [{}], [{}]", entry.getKey(), error);
+ }
+ } else {
+ logger.warn("ignoring persistent setting [{}], not dynamically updateable", entry.getKey());
@tlrx
tlrx Jan 6, 2015 Member

When global cluster state is restored and has many settings, can't this line be too much verbose?

@imotov
imotov Jan 9, 2015 Member

For the setting to be logged here it had to be 1) dynamic when snapshot was made, 2) converted into non-dynamic when restore is made and 3) configured as a persistent setting in snapshotted cluster. Since removal of a dynamic setting is pretty rare event in elasticsearch codebase, nothing should be logged here under normal circumstances.

@tlrx
tlrx Jan 12, 2015 Member

Thanks for the explanation :)

@tlrx
Member
tlrx commented Jan 6, 2015

LGTM left two minor comments

@imotov imotov merged commit a4c92eb into elastic:master Jan 18, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@clintongormley clintongormley removed the review label Mar 19, 2015
@clintongormley clintongormley changed the title from Snapshot/Restore: add validation of restored persistent settings to Add validation of restored persistent settings Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment