From 15b419b97dbd7a8db77c277ed2484c8cfeda62d5 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Wed, 11 Sep 2019 13:48:13 +0200 Subject: [PATCH 1/5] Fixed inconsistent Setting.exist Setting.exist will no longer return true if there's setting in keystore with the same name. Also removed synchronization on keySet to lower congestion, possibly constructing keySet multiple times, which should be cheap though. Closes #41830 --- .../common/settings/Setting.java | 2 +- .../common/settings/Settings.java | 28 +++++++++++-------- .../common/settings/SettingTests.java | 7 +++++ 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index d4164b474de0b..2b19910ae4bbf 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -404,7 +404,7 @@ public T getDefault(Settings settings) { * @return true if the setting is present in the given settings instance, otherwise false */ public boolean exists(final Settings settings) { - return settings.keySet().contains(getKey()); + return settings.keySet(false).contains(getKey()); } /** diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index 0bc5fcba577c8..f315d9c7db79a 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -57,6 +57,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.ListIterator; @@ -66,6 +67,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.function.Predicate; import java.util.function.UnaryOperator; @@ -95,7 +97,7 @@ public final class Settings implements ToXContentFragment { * Setting names found in this Settings for both string and secure settings. * This is constructed lazily in {@link #keySet()}. */ - private final SetOnce> keys = new SetOnce<>(); + private final AtomicReference> keys = new AtomicReference<>(); private Settings(Map settings, SecureSettings secureSettings) { // we use a sorted map for consistent serialization when using getAsMap() @@ -689,21 +691,23 @@ public int size() { } /** Returns the fully qualified setting names contained in this settings object. */ - public Set keySet() { - synchronized (keys) { - if (keys.get() == null) { - if (secureSettings == null) { - keys.set(settings.keySet()); - } else { - Stream stream = Stream.concat(settings.keySet().stream(), secureSettings.getSettingNames().stream()); - // uniquify, since for legacy reasons the same setting name may exist in both - keys.set(Collections.unmodifiableSet(stream.collect(Collectors.toSet()))); - } - } + public Set keySet(boolean includeSecureSettings) { + if (!includeSecureSettings || secureSettings == null) { + return settings.keySet(); + } + if (keys.get() == null) { + Set set = new HashSet<>(settings.keySet()); + set.addAll(secureSettings.getSettingNames()); + keys.compareAndSet(null, Collections.unmodifiableSet(set)); } return keys.get(); } + /** Returns the fully qualified setting names contained in this settings object. */ + public Set keySet() { + return keySet(true); + } + /** * A builder allowing to put different settings and then {@link #build()} an immutable * settings implementation. Use {@link Settings#builder()} in order to diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index b2f73db90f722..5592498e4e810 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -910,6 +910,13 @@ public void testExists() { assertTrue(fooSetting.exists(Settings.builder().put("foo", "bar").build())); } + public void testExistsWithSecure() { + final MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString("foo", "foo"); + Setting fooSetting = Setting.simpleString("foo", Property.NodeScope); + assertFalse(fooSetting.exists(Settings.builder().setSecureSettings(secureSettings).build())); + } + public void testExistsWithFallback() { final int count = randomIntBetween(1, 16); Setting current = Setting.simpleString("fallback0", Property.NodeScope); From af7e590198154597292588c014db5d68b10d416a Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Thu, 12 Sep 2019 11:09:30 +0200 Subject: [PATCH 2/5] Added parameter description --- .../java/org/elasticsearch/common/settings/Settings.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index f315d9c7db79a..c5acf2754a5c5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -690,7 +690,11 @@ public int size() { return keySet().size(); } - /** Returns the fully qualified setting names contained in this settings object. */ + /** + * Returns the fully qualified setting names contained in this settings object. + * + * @param includeSecureSettings if {@code true} then it returns all settings names, only unsecure settings names otherwise + */ public Set keySet(boolean includeSecureSettings) { if (!includeSecureSettings || secureSettings == null) { return settings.keySet(); From 68bb099e87fc6dc96fa1a919dc5e9d6a319293a9 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Thu, 12 Sep 2019 23:39:36 +0200 Subject: [PATCH 3/5] Reuse current methods similar to check in innerGetRaw --- .../common/settings/Setting.java | 3 +- .../common/settings/Settings.java | 40 ++++++++----------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index 2b19910ae4bbf..0c85de72136d8 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -404,7 +404,8 @@ public T getDefault(Settings settings) { * @return true if the setting is present in the given settings instance, otherwise false */ public boolean exists(final Settings settings) { - return settings.keySet(false).contains(getKey()); + SecureSettings secureSettings = settings.getSecureSettings(); + return settings.keySet().contains(getKey()) && (secureSettings == null || !secureSettings.getSettingNames().contains(getKey())); } /** diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index c5acf2754a5c5..ecd1e4ea8161a 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -57,7 +57,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.ListIterator; @@ -67,7 +66,6 @@ import java.util.Set; import java.util.TreeMap; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.function.Predicate; import java.util.function.UnaryOperator; @@ -97,7 +95,7 @@ public final class Settings implements ToXContentFragment { * Setting names found in this Settings for both string and secure settings. * This is constructed lazily in {@link #keySet()}. */ - private final AtomicReference> keys = new AtomicReference<>(); + private final SetOnce> keys = new SetOnce<>(); private Settings(Map settings, SecureSettings secureSettings) { // we use a sorted map for consistent serialization when using getAsMap() @@ -675,7 +673,7 @@ private static void validateValue(String key, Object currentValue, XContentParse public static final Set FORMAT_PARAMS = - Set.of("settings_filter", "flat_settings"); + Set.of("settings_filter", "flat_settings"); /** * Returns {@code true} if this settings object contains no settings @@ -690,26 +688,20 @@ public int size() { return keySet().size(); } - /** - * Returns the fully qualified setting names contained in this settings object. - * - * @param includeSecureSettings if {@code true} then it returns all settings names, only unsecure settings names otherwise - */ - public Set keySet(boolean includeSecureSettings) { - if (!includeSecureSettings || secureSettings == null) { - return settings.keySet(); - } - if (keys.get() == null) { - Set set = new HashSet<>(settings.keySet()); - set.addAll(secureSettings.getSettingNames()); - keys.compareAndSet(null, Collections.unmodifiableSet(set)); - } - return keys.get(); - } - /** Returns the fully qualified setting names contained in this settings object. */ public Set keySet() { - return keySet(true); + synchronized (keys) { + if (keys.get() == null) { + if (secureSettings == null) { + keys.set(settings.keySet()); + } else { + Stream stream = Stream.concat(settings.keySet().stream(), secureSettings.getSettingNames().stream()); + // uniquify, since for legacy reasons the same setting name may exist in both + keys.set(Collections.unmodifiableSet(stream.collect(Collectors.toSet()))); + } + } + } + return keys.get(); } /** @@ -1035,7 +1027,7 @@ private void processLegacyLists(Map map) { */ public Builder loadFromSource(String source, XContentType xContentType) { try (XContentParser parser = XContentFactory.xContent(xContentType) - .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, source)) { + .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, source)) { this.put(fromXContent(parser, true, true)); } catch (Exception e) { throw new SettingsException("Failed to load settings from [" + source + "]", e); @@ -1066,7 +1058,7 @@ public Builder loadFromStream(String resourceName, InputStream is, boolean accep } // fromXContent doesn't use named xcontent or deprecation. try (XContentParser parser = XContentFactory.xContent(xContentType) - .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, is)) { + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, is)) { if (parser.currentToken() == null) { if (parser.nextToken() == null) { return this; // empty file From 53301a2d1b5d036edfd32bf03851f53024ed2171 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Thu, 12 Sep 2019 23:42:11 +0200 Subject: [PATCH 4/5] Revert formatting changes --- .../java/org/elasticsearch/common/settings/Settings.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index ecd1e4ea8161a..0bc5fcba577c8 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -673,7 +673,7 @@ private static void validateValue(String key, Object currentValue, XContentParse public static final Set FORMAT_PARAMS = - Set.of("settings_filter", "flat_settings"); + Set.of("settings_filter", "flat_settings"); /** * Returns {@code true} if this settings object contains no settings @@ -1027,7 +1027,7 @@ private void processLegacyLists(Map map) { */ public Builder loadFromSource(String source, XContentType xContentType) { try (XContentParser parser = XContentFactory.xContent(xContentType) - .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, source)) { + .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, source)) { this.put(fromXContent(parser, true, true)); } catch (Exception e) { throw new SettingsException("Failed to load settings from [" + source + "]", e); @@ -1058,7 +1058,7 @@ public Builder loadFromStream(String resourceName, InputStream is, boolean accep } // fromXContent doesn't use named xcontent or deprecation. try (XContentParser parser = XContentFactory.xContent(xContentType) - .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, is)) { + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, is)) { if (parser.currentToken() == null) { if (parser.nextToken() == null) { return this; // empty file From 1dd83d37b29e1849ad8554309e889c8eb40524e8 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Fri, 13 Sep 2019 15:38:17 +0200 Subject: [PATCH 5/5] Changed ! to ==false --- .../main/java/org/elasticsearch/common/settings/Setting.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index 0c85de72136d8..20c36bb7d5484 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -405,7 +405,8 @@ public T getDefault(Settings settings) { */ public boolean exists(final Settings settings) { SecureSettings secureSettings = settings.getSecureSettings(); - return settings.keySet().contains(getKey()) && (secureSettings == null || !secureSettings.getSettingNames().contains(getKey())); + return settings.keySet().contains(getKey()) && + (secureSettings == null || secureSettings.getSettingNames().contains(getKey()) == false); } /**