From f8248eda61930953b5f40339b98f47d96dc56b66 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 22 Oct 2015 23:06:59 +0200 Subject: [PATCH] apply review comments --- ...ransportNodesListGatewayStartedShards.java | 2 +- .../org/elasticsearch/index/IndexModule.java | 2 +- .../org/elasticsearch/index/IndexService.java | 3 +- .../elasticsearch/index/IndexSettings.java | 19 ++++++----- .../index/IndexSettingsTests.java | 33 ++++++++++++++++--- 5 files changed, 41 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java b/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java index 33740d5528a..3e324d4bea7 100644 --- a/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java +++ b/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java @@ -131,7 +131,7 @@ public class TransportNodesListGatewayStartedShards extends TransportNodesAction if (metaData != null) { ShardPath shardPath = null; try { - IndexSettings indexSettings = new IndexSettings(new Index(metaData.getIndex()), Settings.settingsBuilder().put(settings).put(metaData.getSettings()).build(), Collections.EMPTY_LIST); + IndexSettings indexSettings = new IndexSettings(shardId.index(), Settings.settingsBuilder().put(settings).put(metaData.getSettings()).build(), Collections.EMPTY_LIST); shardPath = ShardPath.loadShardPath(logger, nodeEnv, shardId, indexSettings); if (shardPath == null) { throw new IllegalStateException(shardId + " no shard path found"); diff --git a/core/src/main/java/org/elasticsearch/index/IndexModule.java b/core/src/main/java/org/elasticsearch/index/IndexModule.java index db350bd875a..ed7e0754e34 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexModule.java +++ b/core/src/main/java/org/elasticsearch/index/IndexModule.java @@ -90,7 +90,7 @@ public class IndexModule extends AbstractModule { * Note: an index might be created on a node multiple times. For instance if the last shard from an index is * relocated to another node the internal representation will be destroyed which includes the registered listeners. * Once the node holds at least one shard of an index all modules are reloaded and listeners are registered again. - * Listeners can't be unregistered the will stay alive for the entire time the index is allocated on a node. + * Listeners can't be unregistered they will stay alive for the entire time the index is allocated on a node. *

*/ public void addIndexEventListener(IndexEventListener listener) { diff --git a/core/src/main/java/org/elasticsearch/index/IndexService.java b/core/src/main/java/org/elasticsearch/index/IndexService.java index ec5e52b21b6..0139298fe08 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexService.java +++ b/core/src/main/java/org/elasticsearch/index/IndexService.java @@ -57,7 +57,6 @@ import java.nio.file.Path; import java.util.*; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; import static java.util.Collections.emptyMap; import static java.util.Collections.unmodifiableMap; @@ -509,7 +508,7 @@ public class IndexService extends AbstractIndexComponent implements IndexCompone public synchronized void updateMetaData(final IndexMetaData metadata) { this.indexMetaData = metadata; Settings settings = metadata.getSettings(); - if (this.indexSettings.updateSettings(metadata.getSettings())) { + if (this.indexSettings.updateIndexSettings(metadata.getSettings())) { for (final IndexShard shard : this.shards.values()) { try { shard.onRefreshSettings(settings); diff --git a/core/src/main/java/org/elasticsearch/index/IndexSettings.java b/core/src/main/java/org/elasticsearch/index/IndexSettings.java index 4da6b7433df..afd1f9f97e4 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexSettings.java +++ b/core/src/main/java/org/elasticsearch/index/IndexSettings.java @@ -112,25 +112,26 @@ public final class IndexSettings { * * @return true iff any setting has been updated otherwise false. */ - synchronized boolean updateSettings(Settings settings) { - if (Version.indexCreated(settings) != version) { - throw new IllegalArgumentException("version mismatch on settings update expected: " + version + " but was: " + Version.indexCreated(settings)); + synchronized boolean updateIndexSettings(Settings newSettings) { + if (Version.indexCreated(newSettings) != version) { + throw new IllegalArgumentException("version mismatch on settings update expected: " + version + " but was: " + Version.indexCreated(newSettings)); } - final String newUUID = settings.get(IndexMetaData.SETTING_INDEX_UUID, IndexMetaData.INDEX_UUID_NA_VALUE); + final String newUUID = newSettings.get(IndexMetaData.SETTING_INDEX_UUID, IndexMetaData.INDEX_UUID_NA_VALUE); if (newUUID.equals(getUUID()) == false) { throw new IllegalArgumentException("uuid mismatch on settings update expected: " + uuid + " but was: " + newUUID); } - - if (this.settings.getByPrefix(IndexMetaData.INDEX_SETTING_PREFIX).getAsMap().equals(settings.getByPrefix(IndexMetaData.INDEX_SETTING_PREFIX).getAsMap())) { + final Settings existingSettings = this.settings; + if (existingSettings.getByPrefix(IndexMetaData.INDEX_SETTING_PREFIX).getAsMap().equals(newSettings.getByPrefix(IndexMetaData.INDEX_SETTING_PREFIX).getAsMap())) { // nothing to update, same settings return false; } - this.settings = Settings.builder().put(this.settings).put(settings).build(); + this.settings = Settings.builder().put(existingSettings).put(newSettings).build(); + final Settings mergedSettings = this.settings; for (final Consumer consumer : updateListeners) { try { - consumer.accept(settings); + consumer.accept(mergedSettings); } catch (Exception e) { - logger.warn("failed to refresh index settings for [{}]", e, settings); + logger.warn("failed to refresh index settings for [{}]", e, mergedSettings); } } return true; diff --git a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java b/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java index 2c06e768f0a..56d724ced92 100644 --- a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java @@ -42,13 +42,36 @@ public class IndexSettingsTests extends ESTestCase { assertEquals("0xdeadbeef", settings.getUUID()); assertEquals(1, settings.getUpdateListeners().size()); - assertFalse(settings.updateSettings(theSettings)); + assertFalse(settings.updateIndexSettings(theSettings)); assertSame(theSettings, settings.getSettings()); assertEquals(0, integer.get()); - assertTrue(settings.updateSettings(Settings.builder().put(theSettings).put("index.test.setting.int", 42).build())); + assertTrue(settings.updateIndexSettings(Settings.builder().put(theSettings).put("index.test.setting.int", 42).build())); assertEquals(42, integer.get()); } + public void testMergedSettingsArePassed() { + Version version = VersionUtils.getPreviousVersion(); + Settings theSettings = Settings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, version) + .put(IndexMetaData.SETTING_INDEX_UUID, "0xdeadbeef").build(); + final AtomicInteger integer = new AtomicInteger(0); + final StringBuilder builder = new StringBuilder(); + Consumer settingsConsumer = (s) -> { + integer.set(s.getAsInt("index.test.setting.int", -1)); + builder.append(s.get("not.updated", "")); + }; + IndexSettings settings = new IndexSettings(new Index("index"), theSettings, Collections.singleton(settingsConsumer)); + assertEquals(0, integer.get()); + assertEquals("", builder.toString()); + assertTrue(settings.updateIndexSettings(Settings.builder().put(theSettings).put("index.test.setting.int", 42).build())); + assertEquals(42, integer.get()); + assertEquals("", builder.toString()); + integer.set(0); + assertTrue(settings.updateIndexSettings(Settings.builder().put(theSettings).put("not.updated", "boom").build())); + assertEquals("boom", builder.toString()); + assertEquals(42, integer.get()); + + } + public void testListenerCanThrowException() { Version version = VersionUtils.getPreviousVersion(); Settings theSettings = Settings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, version).put(IndexMetaData.SETTING_INDEX_UUID, "0xdeadbeef").build(); @@ -61,7 +84,7 @@ public class IndexSettingsTests extends ESTestCase { Collections.shuffle(list, random()); IndexSettings settings = new IndexSettings(new Index("index"), theSettings, list); assertEquals(0, integer.get()); - assertTrue(settings.updateSettings(Settings.builder().put(theSettings).put("index.test.setting.int", 42).build())); + assertTrue(settings.updateIndexSettings(Settings.builder().put(theSettings).put("index.test.setting.int", 42).build())); assertEquals(42, integer.get()); } @@ -72,7 +95,7 @@ public class IndexSettingsTests extends ESTestCase { assertEquals(version, settings.getIndexVersionCreated()); assertEquals("_na_", settings.getUUID()); try { - settings.updateSettings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).put("index.test.setting.int", 42).build()); + settings.updateIndexSettings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).put("index.test.setting.int", 42).build()); fail("version has changed"); } catch (IllegalArgumentException ex) { assertTrue(ex.getMessage(), ex.getMessage().startsWith("version mismatch on settings update expected: ")); @@ -81,7 +104,7 @@ public class IndexSettingsTests extends ESTestCase { theSettings = Settings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).put(IndexMetaData.SETTING_INDEX_UUID, "0xdeadbeef").build(); settings = new IndexSettings(new Index("index"), theSettings, Collections.EMPTY_LIST); try { - settings.updateSettings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).put("index.test.setting.int", 42).build()); + settings.updateIndexSettings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).put("index.test.setting.int", 42).build()); fail("uuid missing/change"); } catch (IllegalArgumentException ex) { assertEquals("uuid mismatch on settings update expected: 0xdeadbeef but was: _na_", ex.getMessage());