From 1785df172e0444187d3874f5e728cee4af43bded Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Fri, 14 Oct 2022 03:58:46 -0700 Subject: [PATCH 01/19] =?UTF-8?q?Mapping=C2=A0IllegalArgumentException?= =?UTF-8?q?=C2=A0to=C2=A0InvalidArgumentException=20extend=20OpenSearchExc?= =?UTF-8?q?eption=C2=A0in=20AbstractScopedSettings=20class?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Xue Zhou --- .../opensearch/InvalidArgumentException.java | 28 +++++++++++++++ .../org/opensearch/OpenSearchException.java | 6 ++++ .../settings/AbstractScopedSettings.java | 35 ++++++++++--------- 3 files changed, 52 insertions(+), 17 deletions(-) create mode 100644 server/src/main/java/org/opensearch/InvalidArgumentException.java diff --git a/server/src/main/java/org/opensearch/InvalidArgumentException.java b/server/src/main/java/org/opensearch/InvalidArgumentException.java new file mode 100644 index 0000000000000..cb08752ffcb2b --- /dev/null +++ b/server/src/main/java/org/opensearch/InvalidArgumentException.java @@ -0,0 +1,28 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch; + +import org.opensearch.common.io.stream.StreamInput; + +import java.io.IOException; + +/** + * Base exception for a missing action on a primary + * + * @opensearch.internal + */ +public class InvalidArgumentException extends OpenSearchException{ + public InvalidArgumentException(String message) { + super(message); + } + + public InvalidArgumentException(StreamInput in) throws IOException { + super(in); + } +} diff --git a/server/src/main/java/org/opensearch/OpenSearchException.java b/server/src/main/java/org/opensearch/OpenSearchException.java index 78bda1cf088cd..b32287be384fe 100644 --- a/server/src/main/java/org/opensearch/OpenSearchException.java +++ b/server/src/main/java/org/opensearch/OpenSearchException.java @@ -1613,6 +1613,12 @@ private enum OpenSearchExceptionHandle { org.opensearch.cluster.decommission.NodeDecommissionedException::new, 164, V_3_0_0 + ), + INVALID_ARGUMENT_EXCEPTION( + org.opensearch.InvalidArgumentException.class, + org.opensearch.InvalidArgumentException::new, + 165, + V_3_0_0 ); final Class exceptionClass; diff --git a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java index a75d4f035b790..e0d73a8213be0 100644 --- a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java @@ -38,6 +38,7 @@ import org.apache.lucene.search.spell.LevenshteinDistance; import org.apache.lucene.util.CollectionUtil; import org.opensearch.ExceptionsHelper; +import org.opensearch.InvalidArgumentException; import org.opensearch.common.collect.Tuple; import org.opensearch.common.regex.Regex; @@ -99,7 +100,7 @@ protected AbstractScopedSettings( Map> keySettings = new HashMap<>(); for (Setting setting : settingsSet) { if (setting.getProperties().contains(scope) == false) { - throw new IllegalArgumentException( + throw new InvalidArgumentException( "Setting " + setting + " must be a " + scope + " setting but has: " + setting.getProperties() ); } @@ -108,7 +109,7 @@ protected AbstractScopedSettings( if (setting.hasComplexMatcher()) { Setting overlappingSetting = findOverlappingSetting(setting, complexMatchers); if (overlappingSetting != null) { - throw new IllegalArgumentException( + throw new InvalidArgumentException( "complex setting key: [" + setting.getKey() + "] overlaps existing setting key: [" @@ -129,7 +130,7 @@ protected void validateSettingKey(Setting setting) { if (isValidKey(setting.getKey()) == false && (setting.isGroupSetting() && isValidGroupKey(setting.getKey()) || isValidAffixKey(setting.getKey())) == false || setting.getKey().endsWith(".0")) { - throw new IllegalArgumentException("illegal settings key: [" + setting.getKey() + "]"); + throw new InvalidArgumentException("illegal settings key: [" + setting.getKey() + "]"); } } @@ -231,7 +232,7 @@ public synchronized Settings applySettings(Settings newSettings) { */ public synchronized void addSettingsUpdateConsumer(Setting setting, Consumer consumer, Consumer validator) { if (setting != get(setting.getKey())) { - throw new IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); + throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); } addSettingsUpdater(setting.newUpdater(consumer, logger, validator)); } @@ -394,7 +395,7 @@ public void apply(Map values, Settings current, Settings previ private void ensureSettingIsRegistered(Setting.AffixSetting setting) { final Setting registeredSetting = this.complexMatchers.get(setting.getKey()); if (setting != registeredSetting) { - throw new IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); + throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); } } @@ -410,7 +411,7 @@ public synchronized void addAffixMapUpdateConsumer( ) { final Setting registeredSetting = this.complexMatchers.get(setting.getKey()); if (setting != registeredSetting) { - throw new IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); + throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); } addSettingsUpdater(setting.newAffixMapUpdater(consumer, logger, validator)); } @@ -443,10 +444,10 @@ public synchronized void addSettingsUpdateConsumer( BiConsumer validator ) { if (a != get(a.getKey())) { - throw new IllegalArgumentException("Setting is not registered for key [" + a.getKey() + "]"); + throw new InvalidArgumentException("Setting is not registered for key [" + a.getKey() + "]"); } if (b != get(b.getKey())) { - throw new IllegalArgumentException("Setting is not registered for key [" + b.getKey() + "]"); + throw new InvalidArgumentException("Setting is not registered for key [" + b.getKey() + "]"); } addSettingsUpdater(Setting.compoundUpdater(consumer, validator, a, b, logger)); } @@ -588,7 +589,7 @@ void validate( msg += " please check that any required plugins are installed, or check the breaking changes documentation for removed " + "settings"; } - throw new IllegalArgumentException(msg); + throw new InvalidArgumentException(msg); } else { Set settingsDependencies = setting.getSettingsDependencies(key); if (setting.hasComplexMatcher()) { @@ -605,7 +606,7 @@ void validate( dependency.getKey(), setting.getKey() ); - throw new IllegalArgumentException(message); + throw new InvalidArgumentException(message); } // validate the dependent setting value settingDependency.validate(setting.getKey(), setting.get(settings), dependency.get(settings)); @@ -614,11 +615,11 @@ void validate( // the only time that validateInternalOrPrivateIndex should be true is if this call is coming via the update settings API if (validateInternalOrPrivateIndex) { if (setting.isInternalIndex()) { - throw new IllegalArgumentException( + throw new InvalidArgumentException( "can not update internal setting [" + setting.getKey() + "]; this setting is managed via a dedicated API" ); } else if (setting.isPrivateIndex()) { - throw new IllegalArgumentException( + throw new InvalidArgumentException( "can not update private setting [" + setting.getKey() + "]; this setting is managed by OpenSearch" ); } @@ -765,12 +766,12 @@ public Settings diff(Settings source, Settings defaultSettings) { */ public T get(Setting setting) { if (setting.getProperties().contains(scope) == false) { - throw new IllegalArgumentException( + throw new InvalidArgumentException( "settings scope doesn't match the setting scope [" + this.scope + "] not in [" + setting.getProperties() + "]" ); } if (get(setting.getKey()) == null) { - throw new IllegalArgumentException("setting " + setting.getKey() + " has not been registered"); + throw new InvalidArgumentException("setting " + setting.getKey() + " has not been registered"); } return setting.get(this.lastSettingsApplied, settings); } @@ -843,7 +844,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin toRemove.add(key); // we don't set changed here it's set after we apply deletes below if something actually changed } else if (get(key) == null) { - throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); + throw new InvalidArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { get(key).validateWithoutDependencies(toApply); // we might not have a full picture here do to a dependency validation settingsBuilder.copy(key, toApply); @@ -851,9 +852,9 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin changed |= toApply.get(key).equals(target.get(key)) == false; } else { if (isFinalSetting(key)) { - throw new IllegalArgumentException("final " + type + " setting [" + key + "], not updateable"); + throw new InvalidArgumentException("final " + type + " setting [" + key + "], not updateable"); } else { - throw new IllegalArgumentException(type + " setting [" + key + "], not dynamically updateable"); + throw new InvalidArgumentException(type + " setting [" + key + "], not dynamically updateable"); } } } From 8c9c280c340a83617c32c398af5615d670d90152 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Fri, 14 Oct 2022 04:05:21 -0700 Subject: [PATCH 02/19] Modify changelog Signed-off-by: Xue Zhou --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b29a5d38e7c6..c68cea4c08b93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Backport Apache Lucene version change for 2.4.0 ([#4677](https://github.com/opensearch-project/OpenSearch/pull/4677)) - Refactor Base Action class javadocs to OpenSearch.API ([#4732](https://github.com/opensearch-project/OpenSearch/pull/4732)) - Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459)) +- Mapping IllegalArgumentException to InvalidArgumentException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) ### Deprecated @@ -169,4 +170,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Security [Unreleased]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...HEAD -[2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...2.x \ No newline at end of file +[2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...2.x From 0b6b8274d153db67746210ef05e943fc570a701c Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Fri, 14 Oct 2022 09:58:35 -0700 Subject: [PATCH 03/19] Fix IT class Signed-off-by: Xue Zhou --- .../src/test/java/org/opensearch/client/ClusterClientIT.java | 4 ++-- .../src/test/java/org/opensearch/client/IndicesClientIT.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java index 82d2cbe9149ca..12b117b1354a2 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java @@ -159,10 +159,10 @@ public void testClusterUpdateSettingNonExistent() { highLevelClient().cluster()::putSettingsAsync ) ); - assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST)); + assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); assertThat( exception.getMessage(), - equalTo("OpenSearch exception [type=illegal_argument_exception, reason=transient setting [" + setting + "], not recognized]") + equalTo("OpenSearch exception [type=invalid_argument_exception, reason=transient setting [" + setting + "], not recognized]") ); } diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java index 750b0c15e9c14..44f27391fe83f 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java @@ -1437,7 +1437,7 @@ public void testIndexPutSettings() throws IOException { assertThat( exception.getMessage(), startsWith( - "OpenSearch exception [type=illegal_argument_exception, " + "OpenSearch exception [type=invalid_argument_exception, " + "reason=final index setting [index.number_of_shards], not updateable" ) ); @@ -1471,7 +1471,7 @@ public void testIndexPutSettingNonExistent() throws IOException { highLevelClient().indices()::putSettingsAsync ) ); - assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST)); + assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); assertThat( exception.getMessage(), equalTo( From af93be0d13dc358ceb56975434af34da4fc2bd79 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Fri, 14 Oct 2022 10:50:01 -0700 Subject: [PATCH 04/19] Fix IT class Signed-off-by: Xue Zhou --- .../src/test/java/org/opensearch/client/IndicesClientIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java index 44f27391fe83f..5b5b18d298c8e 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java @@ -1475,7 +1475,7 @@ public void testIndexPutSettingNonExistent() throws IOException { assertThat( exception.getMessage(), equalTo( - "OpenSearch exception [type=illegal_argument_exception, " + "OpenSearch exception [type=invalid_argument_exception, " + "reason=unknown setting [index.no_idea_what_you_are_talking_about] please check that any required plugins are installed, " + "or check the breaking changes documentation for removed settings]" ) From d0d74659e5003e4f2d49a536f4c21e61be5cc0d0 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Fri, 14 Oct 2022 15:12:53 -0700 Subject: [PATCH 05/19] Fix spotless issue Signed-off-by: Xue Zhou --- .../src/main/java/org/opensearch/InvalidArgumentException.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/InvalidArgumentException.java b/server/src/main/java/org/opensearch/InvalidArgumentException.java index cb08752ffcb2b..4d4e07c0c3bbf 100644 --- a/server/src/main/java/org/opensearch/InvalidArgumentException.java +++ b/server/src/main/java/org/opensearch/InvalidArgumentException.java @@ -17,7 +17,7 @@ * * @opensearch.internal */ -public class InvalidArgumentException extends OpenSearchException{ +public class InvalidArgumentException extends OpenSearchException { public InvalidArgumentException(String message) { super(message); } From 6ec459b56aea21c455aa77c7e7f370c68524a4c0 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 19 Oct 2022 10:55:49 -0700 Subject: [PATCH 06/19] delete InvalidArgumentException and change illegalArgumentException to SettingsException Signed-off-by: Xue Zhou --- .../opensearch/client/ClusterClientIT.java | 4 +- .../opensearch/client/IndicesClientIT.java | 7 +-- .../opensearch/InvalidArgumentException.java | 28 --------- .../org/opensearch/OpenSearchException.java | 6 -- .../MetadataIndexTemplateService.java | 5 +- .../settings/AbstractScopedSettings.java | 43 +++++++------- .../common/settings/IndexScopedSettings.java | 4 +- .../opensearch/common/settings/Setting.java | 11 +--- .../common/settings/SettingsException.java | 6 ++ .../MetadataIndexTemplateServiceTests.java | 7 ++- .../common/settings/ScopedSettingsTests.java | 58 +++++++++---------- .../common/settings/SettingsModuleTests.java | 2 +- .../opensearch/index/IndexModuleTests.java | 3 +- .../opensearch/index/IndexSettingsTests.java | 33 ++++++----- .../opensearch/search/SearchServiceTests.java | 5 +- 15 files changed, 95 insertions(+), 127 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/InvalidArgumentException.java diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java index 12b117b1354a2..a955b97b15e34 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/ClusterClientIT.java @@ -159,10 +159,10 @@ public void testClusterUpdateSettingNonExistent() { highLevelClient().cluster()::putSettingsAsync ) ); - assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); + assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST)); assertThat( exception.getMessage(), - equalTo("OpenSearch exception [type=invalid_argument_exception, reason=transient setting [" + setting + "], not recognized]") + equalTo("OpenSearch exception [type=settings_exception, reason=transient setting [" + setting + "], not recognized]") ); } diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java index 5b5b18d298c8e..f92623e989a2e 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/IndicesClientIT.java @@ -1437,8 +1437,7 @@ public void testIndexPutSettings() throws IOException { assertThat( exception.getMessage(), startsWith( - "OpenSearch exception [type=invalid_argument_exception, " - + "reason=final index setting [index.number_of_shards], not updateable" + "OpenSearch exception [type=settings_exception, " + "reason=final index setting [index.number_of_shards], not updateable" ) ); } @@ -1471,11 +1470,11 @@ public void testIndexPutSettingNonExistent() throws IOException { highLevelClient().indices()::putSettingsAsync ) ); - assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR)); + assertThat(exception.status(), equalTo(RestStatus.BAD_REQUEST)); assertThat( exception.getMessage(), equalTo( - "OpenSearch exception [type=invalid_argument_exception, " + "OpenSearch exception [type=settings_exception, " + "reason=unknown setting [index.no_idea_what_you_are_talking_about] please check that any required plugins are installed, " + "or check the breaking changes documentation for removed settings]" ) diff --git a/server/src/main/java/org/opensearch/InvalidArgumentException.java b/server/src/main/java/org/opensearch/InvalidArgumentException.java deleted file mode 100644 index 4d4e07c0c3bbf..0000000000000 --- a/server/src/main/java/org/opensearch/InvalidArgumentException.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch; - -import org.opensearch.common.io.stream.StreamInput; - -import java.io.IOException; - -/** - * Base exception for a missing action on a primary - * - * @opensearch.internal - */ -public class InvalidArgumentException extends OpenSearchException { - public InvalidArgumentException(String message) { - super(message); - } - - public InvalidArgumentException(StreamInput in) throws IOException { - super(in); - } -} diff --git a/server/src/main/java/org/opensearch/OpenSearchException.java b/server/src/main/java/org/opensearch/OpenSearchException.java index b32287be384fe..78bda1cf088cd 100644 --- a/server/src/main/java/org/opensearch/OpenSearchException.java +++ b/server/src/main/java/org/opensearch/OpenSearchException.java @@ -1613,12 +1613,6 @@ private enum OpenSearchExceptionHandle { org.opensearch.cluster.decommission.NodeDecommissionedException::new, 164, V_3_0_0 - ), - INVALID_ARGUMENT_EXCEPTION( - org.opensearch.InvalidArgumentException.class, - org.opensearch.InvalidArgumentException::new, - 165, - V_3_0_0 ); final Class exceptionClass; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java index 7e91b491a234c..ec373db3ce2d9 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java @@ -58,6 +58,7 @@ import org.opensearch.common.regex.Regex; import org.opensearch.common.settings.IndexScopedSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.set.Sets; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -220,7 +221,7 @@ ClusterState addComponentTemplate( ) throws Exception { final ComponentTemplate existing = currentState.metadata().componentTemplates().get(name); if (create && existing != null) { - throw new IllegalArgumentException("component template [" + name + "] already exists"); + throw new SettingsException("component template [" + name + "] already exists"); } CompressedXContent mappings = template.template().mappings(); @@ -256,7 +257,7 @@ ClusterState addComponentTemplate( } } if (globalTemplatesThatUseThisComponent.isEmpty() == false) { - throw new IllegalArgumentException( + throw new SettingsException( "cannot update component template [" + name + "] because the following global templates would resolve to specifying the [" diff --git a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java index e0d73a8213be0..a4ebcf49c41a1 100644 --- a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java @@ -38,7 +38,6 @@ import org.apache.lucene.search.spell.LevenshteinDistance; import org.apache.lucene.util.CollectionUtil; import org.opensearch.ExceptionsHelper; -import org.opensearch.InvalidArgumentException; import org.opensearch.common.collect.Tuple; import org.opensearch.common.regex.Regex; @@ -100,16 +99,14 @@ protected AbstractScopedSettings( Map> keySettings = new HashMap<>(); for (Setting setting : settingsSet) { if (setting.getProperties().contains(scope) == false) { - throw new InvalidArgumentException( - "Setting " + setting + " must be a " + scope + " setting but has: " + setting.getProperties() - ); + throw new SettingsException("Setting " + setting + " must be a " + scope + " setting but has: " + setting.getProperties()); } validateSettingKey(setting); if (setting.hasComplexMatcher()) { Setting overlappingSetting = findOverlappingSetting(setting, complexMatchers); if (overlappingSetting != null) { - throw new InvalidArgumentException( + throw new SettingsException( "complex setting key: [" + setting.getKey() + "] overlaps existing setting key: [" @@ -130,7 +127,7 @@ protected void validateSettingKey(Setting setting) { if (isValidKey(setting.getKey()) == false && (setting.isGroupSetting() && isValidGroupKey(setting.getKey()) || isValidAffixKey(setting.getKey())) == false || setting.getKey().endsWith(".0")) { - throw new InvalidArgumentException("illegal settings key: [" + setting.getKey() + "]"); + throw new SettingsException("illegal settings key: [" + setting.getKey() + "]"); } } @@ -232,7 +229,7 @@ public synchronized Settings applySettings(Settings newSettings) { */ public synchronized void addSettingsUpdateConsumer(Setting setting, Consumer consumer, Consumer validator) { if (setting != get(setting.getKey())) { - throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); + throw new SettingsException("Setting is not registered for key [" + setting.getKey() + "]"); } addSettingsUpdater(setting.newUpdater(consumer, logger, validator)); } @@ -395,7 +392,7 @@ public void apply(Map values, Settings current, Settings previ private void ensureSettingIsRegistered(Setting.AffixSetting setting) { final Setting registeredSetting = this.complexMatchers.get(setting.getKey()); if (setting != registeredSetting) { - throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); + throw new SettingsException("Setting is not registered for key [" + setting.getKey() + "]"); } } @@ -411,7 +408,7 @@ public synchronized void addAffixMapUpdateConsumer( ) { final Setting registeredSetting = this.complexMatchers.get(setting.getKey()); if (setting != registeredSetting) { - throw new InvalidArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); + throw new SettingsException("Setting is not registered for key [" + setting.getKey() + "]"); } addSettingsUpdater(setting.newAffixMapUpdater(consumer, logger, validator)); } @@ -444,10 +441,10 @@ public synchronized void addSettingsUpdateConsumer( BiConsumer validator ) { if (a != get(a.getKey())) { - throw new InvalidArgumentException("Setting is not registered for key [" + a.getKey() + "]"); + throw new SettingsException("Setting is not registered for key [" + a.getKey() + "]"); } if (b != get(b.getKey())) { - throw new InvalidArgumentException("Setting is not registered for key [" + b.getKey() + "]"); + throw new SettingsException("Setting is not registered for key [" + b.getKey() + "]"); } addSettingsUpdater(Setting.compoundUpdater(consumer, validator, a, b, logger)); } @@ -544,7 +541,7 @@ public final void validate( * @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 + * @throws SettingsException if the setting is invalid */ void validate(final String key, final Settings settings, final boolean validateDependencies) { validate(key, settings, validateDependencies, false); @@ -557,7 +554,7 @@ void validate(final String key, final Settings settings, final boolean validateD * @param settings the settings * @param validateDependencies true if dependent settings should be validated * @param validateInternalOrPrivateIndex true if internal index settings should be validated - * @throws IllegalArgumentException if the setting is invalid + * @throws SettingsException if the setting is invalid */ void validate( final String key, @@ -589,7 +586,7 @@ void validate( msg += " please check that any required plugins are installed, or check the breaking changes documentation for removed " + "settings"; } - throw new InvalidArgumentException(msg); + throw new SettingsException(msg); } else { Set settingsDependencies = setting.getSettingsDependencies(key); if (setting.hasComplexMatcher()) { @@ -606,7 +603,7 @@ void validate( dependency.getKey(), setting.getKey() ); - throw new InvalidArgumentException(message); + throw new SettingsException(message); } // validate the dependent setting value settingDependency.validate(setting.getKey(), setting.get(settings), dependency.get(settings)); @@ -615,11 +612,11 @@ void validate( // the only time that validateInternalOrPrivateIndex should be true is if this call is coming via the update settings API if (validateInternalOrPrivateIndex) { if (setting.isInternalIndex()) { - throw new InvalidArgumentException( + throw new SettingsException( "can not update internal setting [" + setting.getKey() + "]; this setting is managed via a dedicated API" ); } else if (setting.isPrivateIndex()) { - throw new InvalidArgumentException( + throw new SettingsException( "can not update private setting [" + setting.getKey() + "]; this setting is managed by OpenSearch" ); } @@ -766,12 +763,12 @@ public Settings diff(Settings source, Settings defaultSettings) { */ public T get(Setting setting) { if (setting.getProperties().contains(scope) == false) { - throw new InvalidArgumentException( + throw new SettingsException( "settings scope doesn't match the setting scope [" + this.scope + "] not in [" + setting.getProperties() + "]" ); } if (get(setting.getKey()) == null) { - throw new InvalidArgumentException("setting " + setting.getKey() + " has not been registered"); + throw new SettingsException("setting " + setting.getKey() + " has not been registered"); } return setting.get(this.lastSettingsApplied, settings); } @@ -780,7 +777,7 @@ public T get(Setting setting) { * Updates a target settings builder with new, updated or deleted settings from a given settings builder. *

* Note: This method will only allow updates to dynamic settings. if a non-dynamic setting is updated an - * {@link IllegalArgumentException} is thrown instead. + * {@link SettingsException} is thrown instead. *

* * @param toApply the new settings to apply @@ -844,7 +841,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin toRemove.add(key); // we don't set changed here it's set after we apply deletes below if something actually changed } else if (get(key) == null) { - throw new InvalidArgumentException(type + " setting [" + key + "], not recognized"); + throw new SettingsException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { get(key).validateWithoutDependencies(toApply); // we might not have a full picture here do to a dependency validation settingsBuilder.copy(key, toApply); @@ -852,9 +849,9 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin changed |= toApply.get(key).equals(target.get(key)) == false; } else { if (isFinalSetting(key)) { - throw new InvalidArgumentException("final " + type + " setting [" + key + "], not updateable"); + throw new SettingsException("final " + type + " setting [" + key + "], not updateable"); } else { - throw new InvalidArgumentException(type + " setting [" + key + "], not dynamically updateable"); + throw new SettingsException(type + " setting [" + key + "], not dynamically updateable"); } } } diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 7be9adc786f24..5fbc49e565b32 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -202,7 +202,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { Map groups = s.getAsGroups(); for (String key : SimilarityService.BUILT_IN.keySet()) { if (groups.containsKey(key)) { - throw new IllegalArgumentException( + throw new SettingsException( "illegal value for [index.similarity." + key + "] cannot redefine built-in similarity" ); } @@ -247,7 +247,7 @@ public IndexScopedSettings copy(Settings settings, IndexMetadata metadata) { @Override protected void validateSettingKey(Setting setting) { if (setting.getKey().startsWith("index.") == false) { - throw new IllegalArgumentException("illegal settings key: [" + setting.getKey() + "] must start with [index.]"); + throw new SettingsException("illegal settings key: [" + setting.getKey() + "] must start with [index.]"); } super.validateSettingKey(setting); } diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index f86fe6771dfcd..bde44f22d2c1f 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -496,15 +496,10 @@ private T get(Settings settings, boolean validate) { } return parsed; } catch (OpenSearchParseException ex) { - throw new IllegalArgumentException(ex.getMessage(), ex); - } catch (NumberFormatException ex) { + throw new SettingsException(ex.getMessage(), ex); + } catch (Exception ex) { String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]"; - throw new IllegalArgumentException(err, ex); - } catch (IllegalArgumentException ex) { - throw ex; - } catch (Exception t) { - String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]"; - throw new IllegalArgumentException(err, t); + throw new SettingsException(err, ex); } } diff --git a/server/src/main/java/org/opensearch/common/settings/SettingsException.java b/server/src/main/java/org/opensearch/common/settings/SettingsException.java index 965255903d23f..cdf9ea11a6932 100644 --- a/server/src/main/java/org/opensearch/common/settings/SettingsException.java +++ b/server/src/main/java/org/opensearch/common/settings/SettingsException.java @@ -34,6 +34,7 @@ import org.opensearch.OpenSearchException; import org.opensearch.common.io.stream.StreamInput; +import org.opensearch.rest.RestStatus; import java.io.IOException; @@ -59,4 +60,9 @@ public SettingsException(StreamInput in) throws IOException { public SettingsException(String msg, Object... args) { super(msg, args); } + + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index 887d8469bd01c..2a70b58d5bd25 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -45,6 +45,7 @@ import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.IndexScopedSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -378,8 +379,8 @@ public void testAddComponentTemplate() throws Exception { assertThat(state.metadata().componentTemplates().get("foo"), equalTo(componentTemplate)); final ClusterState throwState = ClusterState.builder(state).build(); - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + SettingsException e = expectThrows( + SettingsException.class, () -> metadataIndexTemplateService.addComponentTemplate(throwState, true, "foo", componentTemplate) ); assertThat(e.getMessage(), containsString("component template [foo] already exists")); @@ -416,7 +417,7 @@ public void testAddComponentTemplate() throws Exception { ); ComponentTemplate componentTemplate4 = new ComponentTemplate(template, 1L, new HashMap<>()); expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> metadataIndexTemplateService.addComponentTemplate(throwState, true, "foo2", componentTemplate4) ); } diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index c6eb1843d05e1..1f30bd8895bd9 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -80,7 +80,7 @@ public void testResetSetting() { ClusterSettings service = new ClusterSettings(currentSettings, new HashSet<>(Arrays.asList(dynamicSetting, staticSetting))); expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> service.updateDynamicSettings( Settings.builder().put("some.dyn.setting", 8).putNull("some.static.setting").build(), Settings.builder().put(currentSettings), @@ -173,7 +173,7 @@ public void testAddConsumer() { try { service.addSettingsUpdateConsumer(testSetting2, consumer2::set); fail("setting not registered"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals("Setting is not registered for key [foo.bar.baz]", ex.getMessage()); } @@ -183,7 +183,7 @@ public void testAddConsumer() { consumer2.set(b); }); fail("setting not registered"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals("Setting is not registered for key [foo.bar.baz]", ex.getMessage()); } assertEquals(0, consumer.get()); @@ -208,8 +208,8 @@ public void testDependentSettings() { AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(intSetting, stringSetting))); - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + SettingsException iae = expectThrows( + SettingsException.class, () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true) ); assertEquals("missing required setting [foo.test.name] for setting [foo.test.bar]", iae.getMessage()); @@ -288,8 +288,8 @@ public void testDependentSettingsWithFallback() { new HashSet<>(Arrays.asList(nameFallbackSetting, nameSetting, barSetting)) ); - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true) ); assertThat(e, hasToString(containsString("missing required setting [foo.test.name] for setting [foo.test.bar]"))); @@ -963,8 +963,8 @@ public void testGetSetting() { public void testValidateWithSuggestion() { IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + SettingsException iae = expectThrows( + SettingsException.class, () -> settings.validate(Settings.builder().put("index.numbe_of_replica", "1").build(), false) ); assertEquals(iae.getMessage(), "unknown setting [index.numbe_of_replica] did you mean [index.number_of_replicas]?"); @@ -976,26 +976,26 @@ public void testValidate() { + " removed settings"; settings.validate(Settings.builder().put("index.store.type", "boom").build(), false); - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + SettingsException e = expectThrows( + SettingsException.class, () -> settings.validate(Settings.builder().put("index.store.type", "boom").put("i.am.not.a.setting", true).build(), false) ); assertEquals("unknown setting [i.am.not.a.setting]" + unknownMsgSuffix, e.getMessage()); e = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> settings.validate(Settings.builder().put("index.store.type", "boom").put("index.number_of_replicas", true).build(), false) ); assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage()); e = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> settings.validate("index.number_of_replicas", Settings.builder().put("index.number_of_replicas", "true").build(), false) ); assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage()); e = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> settings.validate( "index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build(), @@ -1011,7 +1011,7 @@ public void testValidateSecureSettings() { Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); final ClusterSettings clusterSettings = new ClusterSettings(settings, Collections.emptySet()); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> clusterSettings.validate(settings, false)); + SettingsException e = expectThrows(SettingsException.class, () -> clusterSettings.validate(settings, false)); assertThat(e.getMessage(), startsWith("unknown secure setting [some.secure.setting]")); ClusterSettings clusterSettings2 = new ClusterSettings( @@ -1057,28 +1057,28 @@ public void testKeyPattern() { try { new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.groupSetting("foo.bar.", Property.IndexScope))); fail(); - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { assertEquals("illegal settings key: [foo.bar.] must start with [index.]", e.getMessage()); } try { new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.simpleString("foo.bar", Property.IndexScope))); fail(); - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { assertEquals("illegal settings key: [foo.bar] must start with [index.]", e.getMessage()); } try { new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.groupSetting("index. foo.", Property.IndexScope))); fail(); - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { assertEquals("illegal settings key: [index. foo.]", e.getMessage()); } new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.groupSetting("index.", Property.IndexScope))); try { new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.boolSetting("index.", true, Property.IndexScope))); fail(); - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { assertEquals("illegal settings key: [index.]", e.getMessage()); } new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.boolSetting("index.boo", true, Property.IndexScope))); @@ -1152,7 +1152,7 @@ public void testOverlappingComplexMatchSettings() { try { new ClusterSettings(Settings.EMPTY, settings); fail("an exception should have been thrown because settings overlap"); - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { if (groupFirst) { assertEquals("complex setting key: [foo.bar] overlaps existing setting key: [foo.]", e.getMessage()); } else { @@ -1163,8 +1163,8 @@ public void testOverlappingComplexMatchSettings() { public void testUpdateNumberOfShardsFail() { IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); - IllegalArgumentException ex = expectThrows( - IllegalArgumentException.class, + SettingsException ex = expectThrows( + SettingsException.class, () -> settings.updateSettings( Settings.builder().put("index.number_of_shards", 8).build(), Settings.builder(), @@ -1181,8 +1181,8 @@ public void testFinalSettingUpdateFail() { Settings currentSettings = Settings.builder().put("some.final.setting", 9).put("some.final.group.foo", 7).build(); ClusterSettings service = new ClusterSettings(currentSettings, new HashSet<>(Arrays.asList(finalSetting, finalGroupSetting))); - IllegalArgumentException exc = expectThrows( - IllegalArgumentException.class, + SettingsException exc = expectThrows( + SettingsException.class, () -> service.updateDynamicSettings( Settings.builder().put("some.final.setting", 8).build(), Settings.builder().put(currentSettings), @@ -1193,7 +1193,7 @@ public void testFinalSettingUpdateFail() { assertThat(exc.getMessage(), containsString("final node setting [some.final.setting]")); exc = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> service.updateDynamicSettings( Settings.builder().putNull("some.final.setting").build(), Settings.builder().put(currentSettings), @@ -1204,7 +1204,7 @@ public void testFinalSettingUpdateFail() { assertThat(exc.getMessage(), containsString("final node setting [some.final.setting]")); exc = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> service.updateSettings( Settings.builder().put("some.final.group.new", 8).build(), Settings.builder().put(currentSettings), @@ -1215,7 +1215,7 @@ public void testFinalSettingUpdateFail() { assertThat(exc.getMessage(), containsString("final node setting [some.final.group.new]")); exc = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> service.updateSettings( Settings.builder().put("some.final.group.foo", 5).build(), Settings.builder().put(currentSettings), @@ -1232,7 +1232,7 @@ public void testInternalIndexSettingsFailsValidation() { Settings.EMPTY, Collections.singleton(indexInternalSetting) ); - final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { + final SettingsException e = expectThrows(SettingsException.class, () -> { final Settings settings = Settings.builder().put("index.internal", "internal").build(); indexScopedSettings.validate(settings, false, /* validateInternalOrPrivateIndex */ true); }); @@ -1246,7 +1246,7 @@ public void testPrivateIndexSettingsFailsValidation() { Settings.EMPTY, Collections.singleton(indexInternalSetting) ); - final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { + final SettingsException e = expectThrows(SettingsException.class, () -> { final Settings settings = Settings.builder().put("index.private", "private").build(); indexScopedSettings.validate(settings, false, /* validateInternalOrPrivateIndex */ true); }); diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java index a8306107aaccc..ebd827012f2d1 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java @@ -231,7 +231,7 @@ public void testMutuallyExclusiveScopes() { public void testOldMaxClauseCountSetting() { Settings settings = Settings.builder().put("index.query.bool.max_clause_count", 1024).build(); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new SettingsModule(settings)); + SettingsException ex = expectThrows(SettingsException.class, () -> new SettingsModule(settings)); assertEquals( "unknown setting [index.query.bool.max_clause_count] did you mean [indices.query.bool.max_clause_count]?", ex.getMessage() diff --git a/server/src/test/java/org/opensearch/index/IndexModuleTests.java b/server/src/test/java/org/opensearch/index/IndexModuleTests.java index 6bfdd9ae16773..15399aeeb8bca 100644 --- a/server/src/test/java/org/opensearch/index/IndexModuleTests.java +++ b/server/src/test/java/org/opensearch/index/IndexModuleTests.java @@ -59,6 +59,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.util.BigArrays; import org.opensearch.common.util.PageCacheRecycler; import org.opensearch.common.util.concurrent.OpenSearchRejectedExecutionException; @@ -314,7 +315,7 @@ public void testListener() throws IOException { try { module.addSettingsUpdateConsumer(booleanSetting2, atomicBoolean::set); fail("not registered"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { } diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index de5ef8851ae80..5970d2e3e7e72 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -39,6 +39,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.index.translog.Translog; @@ -601,8 +602,8 @@ public void testPrivateSettingsValidation() { { // validation should fail since we are not ignoring private settings - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean()) ); assertThat(e, hasToString(containsString("unknown setting [index.creation_date]"))); @@ -610,8 +611,8 @@ public void testPrivateSettingsValidation() { { // validation should fail since we are not ignoring private settings - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean(), false, randomBoolean()) ); assertThat(e, hasToString(containsString("unknown setting [index.creation_date]"))); @@ -629,8 +630,8 @@ public void testArchivedSettingsValidation() { { // validation should fail since we are not ignoring archived settings - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean()) ); assertThat(e, hasToString(containsString("unknown setting [archived.foo]"))); @@ -638,8 +639,8 @@ public void testArchivedSettingsValidation() { { // validation should fail since we are not ignoring archived settings - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean(), randomBoolean(), false) ); assertThat(e, hasToString(containsString("unknown setting [archived.foo]"))); @@ -708,8 +709,8 @@ public void testQueryDefaultField() { public void testUpdateSoftDeletesFails() { IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); - IllegalArgumentException error = expectThrows( - IllegalArgumentException.class, + SettingsException error = expectThrows( + SettingsException.class, () -> settings.updateSettings( Settings.builder().put("index.soft_deletes.enabled", randomBoolean()).build(), Settings.builder(), @@ -813,8 +814,8 @@ public void testUpdateRemoteStoreFails() { Set> remoteStoreSettingSet = new HashSet<>(); remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING); IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, remoteStoreSettingSet); - IllegalArgumentException error = expectThrows( - IllegalArgumentException.class, + SettingsException error = expectThrows( + SettingsException.class, () -> settings.updateSettings( Settings.builder().put("index.remote_store.enabled", randomBoolean()).build(), Settings.builder(), @@ -829,8 +830,8 @@ public void testUpdateRemoteTranslogStoreFails() { Set> remoteStoreSettingSet = new HashSet<>(); remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING); IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, remoteStoreSettingSet); - IllegalArgumentException error = expectThrows( - IllegalArgumentException.class, + SettingsException error = expectThrows( + SettingsException.class, () -> settings.updateSettings( Settings.builder().put("index.remote_store.translog.enabled", randomBoolean()).build(), Settings.builder(), @@ -903,8 +904,8 @@ public void testUpdateRemoteRepositoryFails() { Set> remoteStoreSettingSet = new HashSet<>(); remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING); IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, remoteStoreSettingSet); - IllegalArgumentException error = expectThrows( - IllegalArgumentException.class, + SettingsException error = expectThrows( + SettingsException.class, () -> settings.updateSettings( Settings.builder().put("index.remote_store.repository", randomUnicodeOfLength(10)).build(), Settings.builder(), diff --git a/server/src/test/java/org/opensearch/search/SearchServiceTests.java b/server/src/test/java/org/opensearch/search/SearchServiceTests.java index 1f824d40eb638..414c6b4bcb806 100644 --- a/server/src/test/java/org/opensearch/search/SearchServiceTests.java +++ b/server/src/test/java/org/opensearch/search/SearchServiceTests.java @@ -59,6 +59,7 @@ import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.concurrent.OpenSearchRejectedExecutionException; import org.opensearch.common.xcontent.XContentBuilder; @@ -1071,8 +1072,8 @@ public void testSetSearchThrottled() { ) ).actionGet(); - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + SettingsException iae = expectThrows( + SettingsException.class, () -> client().admin() .indices() .prepareUpdateSettings("throttled_threadpool_index") From 982e29f9babba8a96f95a7610cc7fcaafcfdf110 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 19 Oct 2022 21:59:38 -0700 Subject: [PATCH 07/19] fix test failure Signed-off-by: Xue Zhou --- .../azure/classic/AzureSimpleTests.java | 7 +++++-- .../ExampleCustomSettingsConfigTests.java | 9 ++++++--- .../repositories/s3/S3RepositoryTests.java | 15 +++++++++------ .../metadata/EvilSystemPropertyTests.java | 17 +++++++++++------ 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java b/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java index 4c2dda180eda6..95117beb3a0a9 100644 --- a/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java +++ b/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java @@ -36,9 +36,11 @@ import org.opensearch.cloud.azure.classic.management.AzureComputeService.Discovery; import org.opensearch.cloud.azure.classic.management.AzureComputeService.Management; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchIntegTestCase; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, numClientNodes = 0) public class AzureSimpleTests extends AbstractAzureComputeServiceTestCase { @@ -78,7 +80,8 @@ public void testOneNodeShouldRunUsingWrongSettings() { .put(Management.SERVICE_NAME_SETTING.getKey(), "dummy") .put(Discovery.HOST_TYPE_SETTING.getKey(), "do_not_exist"); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNode(settings)); - assertThat(e.getMessage(), containsString("invalid value for host type [do_not_exist]")); + SettingsException e = expectThrows(SettingsException.class, () -> internalCluster().startNode(settings)); + assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); + assertThat(e.getCause().getMessage(), containsString("invalid value for host type [do_not_exist]")); } } diff --git a/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java b/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java index eb1fc056beaff..100edcf0eb79f 100644 --- a/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java +++ b/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java @@ -31,7 +31,9 @@ package org.opensearch.example.customsettings; +import org.hamcrest.Matchers; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import static org.opensearch.example.customsettings.ExampleCustomSettingsConfig.VALIDATED_SETTING; @@ -50,10 +52,11 @@ public void testValidatedSetting() { final String actual = VALIDATED_SETTING.get(Settings.builder().put(VALIDATED_SETTING.getKey(), expected).build()); assertEquals(expected, actual); - final IllegalArgumentException exception = expectThrows( - IllegalArgumentException.class, + final SettingsException exception = expectThrows( + SettingsException.class, () -> VALIDATED_SETTING.get(Settings.builder().put("custom.validated", "it's forbidden").build()) ); - assertEquals("Setting must not contain [forbidden]", exception.getMessage()); + assertThat(exception.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); + assertEquals("Setting must not contain [forbidden]", exception.getCause().getMessage()); } } diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java index da1f5d71b4da5..9413e96070e63 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java @@ -36,6 +36,7 @@ import org.opensearch.cluster.metadata.RepositoryMetadata; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -88,17 +89,19 @@ public void testInvalidChunkBufferSizeSettings() { createS3Repo(getRepositoryMetadata(s3)).close(); // buffer < 5mb should fail final Settings s4 = bufferAndChunkSettings(4, 10); - final IllegalArgumentException e2 = expectThrows( - IllegalArgumentException.class, + final SettingsException e2 = expectThrows( + SettingsException.class, () -> createS3Repo(getRepositoryMetadata(s4)).close() ); - assertThat(e2.getMessage(), containsString("failed to parse value [4mb] for setting [buffer_size], must be >= [5mb]")); + assertThat(e2.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); + assertThat(e2.getCause().getMessage(), containsString("failed to parse value [4mb] for setting [buffer_size], must be >= [5mb]")); final Settings s5 = bufferAndChunkSettings(5, 6000000); - final IllegalArgumentException e3 = expectThrows( - IllegalArgumentException.class, + final SettingsException e3 = expectThrows( + SettingsException.class, () -> createS3Repo(getRepositoryMetadata(s5)).close() ); - assertThat(e3.getMessage(), containsString("failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]")); + assertThat(e3.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); + assertThat(e3.getCause().getMessage(), containsString("failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]")); } private Settings bufferAndChunkSettings(long buffer, long chunk) { diff --git a/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java b/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java index ddb7913b0a1ed..aa9a3600a168c 100644 --- a/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java +++ b/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java @@ -31,8 +31,10 @@ package org.opensearch.cluster.metadata; +import org.hamcrest.Matchers; import org.opensearch.common.SuppressForbidden; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import static org.opensearch.cluster.metadata.IndexMetadata.DEFAULT_NUMBER_OF_SHARDS; @@ -43,20 +45,22 @@ public class EvilSystemPropertyTests extends OpenSearchTestCase { @SuppressForbidden(reason = "manipulates system properties for testing") public void testNumShards() { - IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> + SettingsException exception = expectThrows(SettingsException.class, () -> IndexMetadata.buildNumberOfShardsSetting() .get(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 1025).build())); - assertEquals("Failed to parse value [1025] for setting [" + SETTING_NUMBER_OF_SHARDS + "] must be <= 1024", exception.getMessage()); + assertEquals("Failed to parse value [1025] for setting [" + SETTING_NUMBER_OF_SHARDS + "] must be <= 1024", exception.getCause().getMessage()); Integer numShards = IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 100).build()); assertEquals(100, numShards.intValue()); int limit = randomIntBetween(1, 10); System.setProperty(MAX_NUMBER_OF_SHARDS, Integer.toString(limit)); try { - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + SettingsException e = expectThrows(SettingsException.class, () -> IndexMetadata.buildNumberOfShardsSetting() .get(Settings.builder().put("index.number_of_shards", 11).build())); - assertEquals("Failed to parse value [11] for setting [index.number_of_shards] must be <= " + limit, e.getMessage()); + Throwable cause = e.getCause(); + assertThat(cause, Matchers.instanceOf(IllegalArgumentException.class)); + assertEquals("Failed to parse value [11] for setting [index.number_of_shards] must be <= " + limit, cause.getMessage()); System.clearProperty(MAX_NUMBER_OF_SHARDS); Integer defaultFromSetting = IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getDefault(Settings.EMPTY); @@ -70,9 +74,10 @@ public void testNumShards() { randomDefault = randomIntBetween(1, 10); System.setProperty(MAX_NUMBER_OF_SHARDS, Integer.toString(randomDefault)); System.setProperty(DEFAULT_NUMBER_OF_SHARDS, Integer.toString(randomDefault + 1)); - e = expectThrows(IllegalArgumentException.class, IndexMetadata::buildNumberOfShardsSetting); + + cause = expectThrows(IllegalArgumentException.class, IndexMetadata::buildNumberOfShardsSetting); assertEquals(DEFAULT_NUMBER_OF_SHARDS + " value [" + (randomDefault + 1) + "] must between " + - "1 and " + MAX_NUMBER_OF_SHARDS + " [" + randomDefault + "]", e.getMessage()); + "1 and " + MAX_NUMBER_OF_SHARDS + " [" + randomDefault + "]", cause.getMessage()); } finally { System.clearProperty(MAX_NUMBER_OF_SHARDS); System.clearProperty(DEFAULT_NUMBER_OF_SHARDS); From a210be7c664207d01f25f240e9ab85f6c44917bf Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 19 Oct 2022 22:55:01 -0700 Subject: [PATCH 08/19] Fix spotless issue Signed-off-by: Xue Zhou --- .../repositories/s3/S3RepositoryTests.java | 15 ++++++--------- .../org/opensearch/common/settings/Setting.java | 2 +- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java index 9413e96070e63..b48eb81be56cf 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java @@ -89,19 +89,16 @@ public void testInvalidChunkBufferSizeSettings() { createS3Repo(getRepositoryMetadata(s3)).close(); // buffer < 5mb should fail final Settings s4 = bufferAndChunkSettings(4, 10); - final SettingsException e2 = expectThrows( - SettingsException.class, - () -> createS3Repo(getRepositoryMetadata(s4)).close() - ); + final SettingsException e2 = expectThrows(SettingsException.class, () -> createS3Repo(getRepositoryMetadata(s4)).close()); assertThat(e2.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); assertThat(e2.getCause().getMessage(), containsString("failed to parse value [4mb] for setting [buffer_size], must be >= [5mb]")); final Settings s5 = bufferAndChunkSettings(5, 6000000); - final SettingsException e3 = expectThrows( - SettingsException.class, - () -> createS3Repo(getRepositoryMetadata(s5)).close() - ); + final SettingsException e3 = expectThrows(SettingsException.class, () -> createS3Repo(getRepositoryMetadata(s5)).close()); assertThat(e3.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); - assertThat(e3.getCause().getMessage(), containsString("failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]")); + assertThat( + e3.getCause().getMessage(), + containsString("failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]") + ); } private Settings bufferAndChunkSettings(long buffer, long chunk) { diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index bde44f22d2c1f..9dd2c34907a8c 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -497,7 +497,7 @@ private T get(Settings settings, boolean validate) { return parsed; } catch (OpenSearchParseException ex) { throw new SettingsException(ex.getMessage(), ex); - } catch (Exception ex) { + } catch (Exception ex) { String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]"; throw new SettingsException(err, ex); } From c1daf2414d1d65bbc5431e6ef80480c4129e2e17 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Thu, 20 Oct 2022 12:38:43 -0700 Subject: [PATCH 09/19] Fix test failure Signed-off-by: Xue Zhou --- .../azure/AzureRepositorySettingsTests.java | 17 ++++++++++------- .../15_connection_mode_configuration.yml | 10 +++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java index 01235d6193d9c..d5fe1fcd073c3 100644 --- a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java +++ b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java @@ -32,6 +32,7 @@ package org.opensearch.repositories.azure; +import org.opensearch.common.settings.SettingsException; import reactor.core.scheduler.Schedulers; import org.junit.AfterClass; @@ -46,6 +47,7 @@ import org.opensearch.repositories.blobstore.BlobStoreTestUtil; import org.opensearch.test.OpenSearchTestCase; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.mockito.Mockito.mock; @@ -161,21 +163,22 @@ public void testChunkSize() { assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), azureRepository.chunkSize()); // zero bytes is not allowed - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + SettingsException e = expectThrows( + SettingsException.class, () -> azureRepository(Settings.builder().put("chunk_size", "0").build()) ); - assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getMessage()); + assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); + assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); // negative bytes not allowed - e = expectThrows(IllegalArgumentException.class, () -> azureRepository(Settings.builder().put("chunk_size", "-1").build())); - assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getMessage()); + e = expectThrows(SettingsException.class, () -> azureRepository(Settings.builder().put("chunk_size", "-1").build())); + assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); // greater than max chunk size not allowed - e = expectThrows(IllegalArgumentException.class, () -> azureRepository(Settings.builder().put("chunk_size", "6tb").build())); + e = expectThrows(SettingsException.class, () -> azureRepository(Settings.builder().put("chunk_size", "6tb").build())); assertEquals( "failed to parse value [6tb] for setting [chunk_size], must be <= [" + AzureStorageService.MAX_CHUNK_SIZE.getStringRep() + "]", - e.getMessage() + e.getCause().getMessage() ); } diff --git a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml index 05185cb3e3328..660cca021920b 100644 --- a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml +++ b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml @@ -17,7 +17,7 @@ cluster.remote.test_remote_cluster.proxy_address: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.type: "settings_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.node_connections\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=SNIFF, configured=PROXY]" } @@ -32,7 +32,7 @@ cluster.remote.test_remote_cluster.proxy_address: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.type: "settings_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.seeds\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=SNIFF, configured=PROXY]" } @@ -54,7 +54,7 @@ cluster.remote.test_remote_cluster.seeds: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.type: "settings_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.proxy_socket_connections\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=PROXY, configured=SNIFF]" } @@ -68,7 +68,7 @@ cluster.remote.test_remote_cluster.seeds: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.type: "settings_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.proxy_address\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=PROXY, configured=SNIFF]" } @@ -182,7 +182,7 @@ cluster.remote.test_remote_cluster.proxy_address: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.type: "settings_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.seeds\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=SNIFF, configured=PROXY]" } From 1f47ec5a884ac59b65472c8aacfe25ba6a940580 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 9 Nov 2022 13:42:32 -0800 Subject: [PATCH 10/19] rebase and fix changelog conflicts Signed-off-by: Xue Zhou --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61fdb6c52c9c2..98b5c21455e54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -129,7 +129,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Refactored BalancedAllocator.Balancer to LocalShardsBalancer ([#4761](https://github.com/opensearch-project/OpenSearch/pull/4761)) - Fail weight update when decommission ongoing and fail decommission when attribute not weighed away ([#4839](https://github.com/opensearch-project/OpenSearch/pull/4839)) - Skip SymbolicLinkPreservingTarIT when running on Windows ([#5023](https://github.com/opensearch-project/OpenSearch/pull/5023)) -- Mapping IllegalArgumentException to InvalidArgumentException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) +- Mapping IllegalArgumentException to SettingsException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) ### Deprecated ### Removed From d7af0d43dd600c47d692bb15a85bfda1840e3ed7 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 9 Nov 2022 17:11:54 -0800 Subject: [PATCH 11/19] fix test failure Signed-off-by: Xue Zhou --- .../azure/AzureRepositorySettingsTests.java | 17 ++++----- ...eCloudStorageBlobStoreRepositoryTests.java | 13 ++++--- .../repositories/s3/S3RepositoryTests.java | 18 ++++----- .../metadata/EvilSystemPropertyTests.java | 15 +++----- .../15_connection_mode_configuration.yml | 10 ++--- .../MetadataIndexTemplateService.java | 5 +-- .../common/settings/IndexScopedSettings.java | 4 +- .../opensearch/common/settings/Setting.java | 11 ++++-- .../settings/SettingsUpdaterTests.java | 7 ++-- .../action/support/AutoCreateIndexTests.java | 9 +++-- .../ElectionSchedulerFactoryTests.java | 37 +++++++++++++------ .../NoClusterManagerBlockServiceTests.java | 3 +- .../metadata/AutoExpandReplicasTests.java | 11 +++--- .../cluster/metadata/IndexMetadataTests.java | 28 +++++--------- .../MetadataIndexTemplateServiceTests.java | 4 +- .../DiskThresholdSettingsTests.java | 6 +-- .../common/settings/ScopedSettingsTests.java | 6 +-- 17 files changed, 106 insertions(+), 98 deletions(-) diff --git a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java index d5fe1fcd073c3..01235d6193d9c 100644 --- a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java +++ b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java @@ -32,7 +32,6 @@ package org.opensearch.repositories.azure; -import org.opensearch.common.settings.SettingsException; import reactor.core.scheduler.Schedulers; import org.junit.AfterClass; @@ -47,7 +46,6 @@ import org.opensearch.repositories.blobstore.BlobStoreTestUtil; import org.opensearch.test.OpenSearchTestCase; -import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.mockito.Mockito.mock; @@ -163,22 +161,21 @@ public void testChunkSize() { assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), azureRepository.chunkSize()); // zero bytes is not allowed - SettingsException e = expectThrows( - SettingsException.class, + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, () -> azureRepository(Settings.builder().put("chunk_size", "0").build()) ); - assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); - assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); + assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getMessage()); // negative bytes not allowed - e = expectThrows(SettingsException.class, () -> azureRepository(Settings.builder().put("chunk_size", "-1").build())); - assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); + e = expectThrows(IllegalArgumentException.class, () -> azureRepository(Settings.builder().put("chunk_size", "-1").build())); + assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getMessage()); // greater than max chunk size not allowed - e = expectThrows(SettingsException.class, () -> azureRepository(Settings.builder().put("chunk_size", "6tb").build())); + e = expectThrows(IllegalArgumentException.class, () -> azureRepository(Settings.builder().put("chunk_size", "6tb").build())); assertEquals( "failed to parse value [6tb] for setting [chunk_size], must be <= [" + AzureStorageService.MAX_CHUNK_SIZE.getStringRep() + "]", - e.getCause().getMessage() + e.getMessage() ); } diff --git a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java index c0a396724897f..c43470871e76a 100644 --- a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java +++ b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java @@ -55,6 +55,7 @@ import org.opensearch.common.regex.Regex; import org.opensearch.common.settings.MockSecureSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -163,7 +164,7 @@ public void testChunkSize() { assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), chunkSize); // zero bytes is not allowed - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { + SettingsException e = expectThrows(SettingsException.class, () -> { final RepositoryMetadata repoMetadata = new RepositoryMetadata( "repo", GoogleCloudStorageRepository.TYPE, @@ -171,10 +172,10 @@ public void testChunkSize() { ); GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetadata); }); - assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getMessage()); + assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); // negative bytes not allowed - e = expectThrows(IllegalArgumentException.class, () -> { + e = expectThrows(SettingsException.class, () -> { final RepositoryMetadata repoMetadata = new RepositoryMetadata( "repo", GoogleCloudStorageRepository.TYPE, @@ -182,10 +183,10 @@ public void testChunkSize() { ); GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetadata); }); - assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getMessage()); + assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); // greater than max chunk size not allowed - e = expectThrows(IllegalArgumentException.class, () -> { + e = expectThrows(SettingsException.class, () -> { final RepositoryMetadata repoMetadata = new RepositoryMetadata( "repo", GoogleCloudStorageRepository.TYPE, @@ -193,7 +194,7 @@ public void testChunkSize() { ); GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetadata); }); - assertEquals("failed to parse value [6tb] for setting [chunk_size], must be <= [5tb]", e.getMessage()); + assertEquals("failed to parse value [6tb] for setting [chunk_size], must be <= [5tb]", e.getCause().getMessage()); } public void testWriteReadLarge() throws IOException { diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java index b48eb81be56cf..da1f5d71b4da5 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java @@ -36,7 +36,6 @@ import org.opensearch.cluster.metadata.RepositoryMetadata; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -89,16 +88,17 @@ public void testInvalidChunkBufferSizeSettings() { createS3Repo(getRepositoryMetadata(s3)).close(); // buffer < 5mb should fail final Settings s4 = bufferAndChunkSettings(4, 10); - final SettingsException e2 = expectThrows(SettingsException.class, () -> createS3Repo(getRepositoryMetadata(s4)).close()); - assertThat(e2.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); - assertThat(e2.getCause().getMessage(), containsString("failed to parse value [4mb] for setting [buffer_size], must be >= [5mb]")); + final IllegalArgumentException e2 = expectThrows( + IllegalArgumentException.class, + () -> createS3Repo(getRepositoryMetadata(s4)).close() + ); + assertThat(e2.getMessage(), containsString("failed to parse value [4mb] for setting [buffer_size], must be >= [5mb]")); final Settings s5 = bufferAndChunkSettings(5, 6000000); - final SettingsException e3 = expectThrows(SettingsException.class, () -> createS3Repo(getRepositoryMetadata(s5)).close()); - assertThat(e3.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); - assertThat( - e3.getCause().getMessage(), - containsString("failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]") + final IllegalArgumentException e3 = expectThrows( + IllegalArgumentException.class, + () -> createS3Repo(getRepositoryMetadata(s5)).close() ); + assertThat(e3.getMessage(), containsString("failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]")); } private Settings bufferAndChunkSettings(long buffer, long chunk) { diff --git a/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java b/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java index aa9a3600a168c..869a84c1bbc02 100644 --- a/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java +++ b/qa/evil-tests/src/test/java/org/opensearch/cluster/metadata/EvilSystemPropertyTests.java @@ -45,22 +45,20 @@ public class EvilSystemPropertyTests extends OpenSearchTestCase { @SuppressForbidden(reason = "manipulates system properties for testing") public void testNumShards() { - SettingsException exception = expectThrows(SettingsException.class, () -> + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> IndexMetadata.buildNumberOfShardsSetting() .get(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 1025).build())); - assertEquals("Failed to parse value [1025] for setting [" + SETTING_NUMBER_OF_SHARDS + "] must be <= 1024", exception.getCause().getMessage()); + assertEquals("Failed to parse value [1025] for setting [" + SETTING_NUMBER_OF_SHARDS + "] must be <= 1024", exception.getMessage()); Integer numShards = IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 100).build()); assertEquals(100, numShards.intValue()); int limit = randomIntBetween(1, 10); System.setProperty(MAX_NUMBER_OF_SHARDS, Integer.toString(limit)); try { - SettingsException e = expectThrows(SettingsException.class, () -> + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> IndexMetadata.buildNumberOfShardsSetting() .get(Settings.builder().put("index.number_of_shards", 11).build())); - Throwable cause = e.getCause(); - assertThat(cause, Matchers.instanceOf(IllegalArgumentException.class)); - assertEquals("Failed to parse value [11] for setting [index.number_of_shards] must be <= " + limit, cause.getMessage()); + assertEquals("Failed to parse value [11] for setting [index.number_of_shards] must be <= " + limit, e.getMessage()); System.clearProperty(MAX_NUMBER_OF_SHARDS); Integer defaultFromSetting = IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getDefault(Settings.EMPTY); @@ -74,10 +72,9 @@ public void testNumShards() { randomDefault = randomIntBetween(1, 10); System.setProperty(MAX_NUMBER_OF_SHARDS, Integer.toString(randomDefault)); System.setProperty(DEFAULT_NUMBER_OF_SHARDS, Integer.toString(randomDefault + 1)); - - cause = expectThrows(IllegalArgumentException.class, IndexMetadata::buildNumberOfShardsSetting); + e = expectThrows(IllegalArgumentException.class, IndexMetadata::buildNumberOfShardsSetting); assertEquals(DEFAULT_NUMBER_OF_SHARDS + " value [" + (randomDefault + 1) + "] must between " + - "1 and " + MAX_NUMBER_OF_SHARDS + " [" + randomDefault + "]", cause.getMessage()); + "1 and " + MAX_NUMBER_OF_SHARDS + " [" + randomDefault + "]", e.getMessage()); } finally { System.clearProperty(MAX_NUMBER_OF_SHARDS); System.clearProperty(DEFAULT_NUMBER_OF_SHARDS); diff --git a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml index 660cca021920b..05185cb3e3328 100644 --- a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml +++ b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/15_connection_mode_configuration.yml @@ -17,7 +17,7 @@ cluster.remote.test_remote_cluster.proxy_address: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "settings_exception" } + - match: { error.root_cause.0.type: "illegal_argument_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.node_connections\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=SNIFF, configured=PROXY]" } @@ -32,7 +32,7 @@ cluster.remote.test_remote_cluster.proxy_address: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "settings_exception" } + - match: { error.root_cause.0.type: "illegal_argument_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.seeds\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=SNIFF, configured=PROXY]" } @@ -54,7 +54,7 @@ cluster.remote.test_remote_cluster.seeds: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "settings_exception" } + - match: { error.root_cause.0.type: "illegal_argument_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.proxy_socket_connections\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=PROXY, configured=SNIFF]" } @@ -68,7 +68,7 @@ cluster.remote.test_remote_cluster.seeds: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "settings_exception" } + - match: { error.root_cause.0.type: "illegal_argument_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.proxy_address\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=PROXY, configured=SNIFF]" } @@ -182,7 +182,7 @@ cluster.remote.test_remote_cluster.proxy_address: $remote_ip - match: { status: 400 } - - match: { error.root_cause.0.type: "settings_exception" } + - match: { error.root_cause.0.type: "illegal_argument_exception" } - match: { error.root_cause.0.reason: "Setting \"cluster.remote.test_remote_cluster.seeds\" cannot be used with the configured \"cluster.remote.test_remote_cluster.mode\" [required=SNIFF, configured=PROXY]" } diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java index efd6af17296c3..c2160b37f2722 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java @@ -60,7 +60,6 @@ import org.opensearch.common.regex.Regex; import org.opensearch.common.settings.IndexScopedSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.set.Sets; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -253,7 +252,7 @@ ClusterState addComponentTemplate( ) throws Exception { final ComponentTemplate existing = currentState.metadata().componentTemplates().get(name); if (create && existing != null) { - throw new SettingsException("component template [" + name + "] already exists"); + throw new IllegalArgumentException("component template [" + name + "] already exists"); } CompressedXContent mappings = template.template().mappings(); @@ -289,7 +288,7 @@ ClusterState addComponentTemplate( } } if (globalTemplatesThatUseThisComponent.isEmpty() == false) { - throw new SettingsException( + throw new IllegalArgumentException( "cannot update component template [" + name + "] because the following global templates would resolve to specifying the [" diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index cd8adf7963b25..079fc38415328 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -202,7 +202,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { Map groups = s.getAsGroups(); for (String key : SimilarityService.BUILT_IN.keySet()) { if (groups.containsKey(key)) { - throw new SettingsException( + throw new IllegalArgumentException( "illegal value for [index.similarity." + key + "] cannot redefine built-in similarity" ); } @@ -254,7 +254,7 @@ public IndexScopedSettings copy(Settings settings, IndexMetadata metadata) { @Override protected void validateSettingKey(Setting setting) { if (setting.getKey().startsWith("index.") == false) { - throw new SettingsException("illegal settings key: [" + setting.getKey() + "] must start with [index.]"); + throw new IllegalArgumentException("illegal settings key: [" + setting.getKey() + "] must start with [index.]"); } super.validateSettingKey(setting); } diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index 9dd2c34907a8c..f86fe6771dfcd 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -496,10 +496,15 @@ private T get(Settings settings, boolean validate) { } return parsed; } catch (OpenSearchParseException ex) { - throw new SettingsException(ex.getMessage(), ex); - } catch (Exception ex) { + throw new IllegalArgumentException(ex.getMessage(), ex); + } catch (NumberFormatException ex) { String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]"; - throw new SettingsException(err, ex); + throw new IllegalArgumentException(err, ex); + } catch (IllegalArgumentException ex) { + throw ex; + } catch (Exception t) { + String err = "Failed to parse value" + (isFiltered() ? "" : " [" + value + "]") + " for setting [" + getKey() + "]"; + throw new IllegalArgumentException(err, t); } } diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 8d65c24f36b63..48b50813929f5 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -39,6 +39,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; @@ -157,7 +158,7 @@ public void testAllOrNothing() { logger ); fail("all or nothing"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { logger.info("", ex); assertEquals("Failed to parse value [not a float] for setting [cluster.routing.allocation.balance.index]", ex.getMessage()); } @@ -663,11 +664,11 @@ public void testUpdateOfValidationDependentSettings() { final ClusterState finalCluster = cluster; Exception exception = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> updater.updateSettings(finalCluster, Settings.builder().put(SETTING_FOO_HIGH.getKey(), 2).build(), Settings.EMPTY, logger) ); - assertThat(exception.getMessage(), equalTo("[high]=2 is lower than [low]=5")); + assertThat(exception.getCause().getMessage(), equalTo("[high]=2 is lower than [low]=5")); } } diff --git a/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java b/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java index 068414678e860..818c3c645f46d 100644 --- a/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java +++ b/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java @@ -41,6 +41,7 @@ import org.opensearch.common.collect.Tuple; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.index.IndexNotFoundException; import org.opensearch.indices.SystemIndexDescriptor; @@ -62,11 +63,11 @@ public void testParseFailed() { Settings settings = Settings.builder().put("action.auto_create_index", ",,,").build(); newAutoCreateIndex(settings); fail("initialization should have failed"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals( "Can't parse [,,,] for setting [action.auto_create_index] must be either [true, false, or a " + "comma separated list of index patterns]", - ex.getMessage() + ex.getCause().getMessage() ); } } @@ -77,10 +78,10 @@ public void testParseFailedMissingIndex() { try { newAutoCreateIndex(settings); fail("initialization should have failed"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals( "Can't parse [" + prefix + "] for setting [action.auto_create_index] must contain an index name after [" + prefix + "]", - ex.getMessage() + ex.getCause().getMessage() ); } } diff --git a/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java b/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java index 164f802fdae38..022a665b24bfc 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java @@ -35,6 +35,7 @@ import org.opensearch.common.lease.Releasable; import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.Settings.Builder; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.test.OpenSearchTestCase; @@ -180,44 +181,56 @@ public void testRetriesOnCorrectSchedule() { public void testSettingsValidation() { { final Settings settings = Settings.builder().put(ELECTION_INITIAL_TIMEOUT_SETTING.getKey(), "0s").build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); - assertThat(e.getMessage(), is("failed to parse value [0s] for setting [cluster.election.initial_timeout], must be >= [1ms]")); + SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); + assertThat( + e.getCause().getMessage(), + is("failed to parse value [0s] for setting [cluster.election.initial_timeout], must be >= [1ms]") + ); } { final Settings settings = Settings.builder().put(ELECTION_INITIAL_TIMEOUT_SETTING.getKey(), "10001ms").build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); + SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); assertThat( - e.getMessage(), + e.getCause().getMessage(), is("failed to parse value [10001ms] for setting [cluster.election.initial_timeout], must be <= [10s]") ); } { final Settings settings = Settings.builder().put(ELECTION_BACK_OFF_TIME_SETTING.getKey(), "0s").build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); - assertThat(e.getMessage(), is("failed to parse value [0s] for setting [cluster.election.back_off_time], must be >= [1ms]")); + SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); + assertThat( + e.getCause().getMessage(), + is("failed to parse value [0s] for setting [cluster.election.back_off_time], must be >= [1ms]") + ); } { final Settings settings = Settings.builder().put(ELECTION_BACK_OFF_TIME_SETTING.getKey(), "60001ms").build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); + SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); assertThat( - e.getMessage(), + e.getCause().getMessage(), is("failed to parse value [60001ms] for setting [cluster.election.back_off_time], must be <= [60s]") ); } { final Settings settings = Settings.builder().put(ELECTION_MAX_TIMEOUT_SETTING.getKey(), "199ms").build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); - assertThat(e.getMessage(), is("failed to parse value [199ms] for setting [cluster.election.max_timeout], must be >= [200ms]")); + SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); + assertThat( + e.getCause().getMessage(), + is("failed to parse value [199ms] for setting [cluster.election.max_timeout], must be >= [200ms]") + ); } { final Settings settings = Settings.builder().put(ELECTION_MAX_TIMEOUT_SETTING.getKey(), "301s").build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); - assertThat(e.getMessage(), is("failed to parse value [301s] for setting [cluster.election.max_timeout], must be <= [300s]")); + SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); + assertThat( + e.getCause().getMessage(), + is("failed to parse value [301s] for setting [cluster.election.max_timeout], must be <= [300s]") + ); } { diff --git a/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java b/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java index 7bd2fefe50c42..515d868a60206 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java @@ -33,6 +33,7 @@ import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import static org.opensearch.cluster.coordination.NoClusterManagerBlockService.NO_CLUSTER_MANAGER_BLOCK_ALL; @@ -74,7 +75,7 @@ public void testBlocksMetadataWritesIfConfiguredBySetting() { public void testRejectsInvalidSetting() { expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> createService(Settings.builder().put(NO_CLUSTER_MANAGER_BLOCK_SETTING.getKey(), "unknown").build()) ); } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java b/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java index 4b7eaf0272a91..787853262c8d7 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java @@ -44,6 +44,7 @@ import org.opensearch.cluster.routing.IndexShardRoutingTable; import org.opensearch.cluster.routing.ShardRoutingState; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.indices.cluster.ClusterStateChanges; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; @@ -92,29 +93,29 @@ public void testInvalidValues() { try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "boom").build()); fail(); - } catch (IllegalArgumentException ex) { - assertEquals("failed to parse [index.auto_expand_replicas] from value: [boom] at index -1", ex.getMessage()); + } catch (SettingsException ex) { + assertEquals("failed to parse [index.auto_expand_replicas] from value: [boom] at index -1", ex.getCause().getMessage()); } try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "1-boom").build()); fail(); } catch (IllegalArgumentException ex) { - assertEquals("failed to parse [index.auto_expand_replicas] from value: [1-boom] at index 1", ex.getMessage()); + assertEquals("failed to parse [index.auto_expand_replicas] from value: [1-boom] at index 1", ex.getCause().getMessage()); assertEquals("For input string: \"boom\"", ex.getCause().getMessage()); } try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "boom-1").build()); fail(); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals("failed to parse [index.auto_expand_replicas] from value: [boom-1] at index 4", ex.getMessage()); assertEquals("For input string: \"boom\"", ex.getCause().getMessage()); } try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "2-1").build()); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals("[index.auto_expand_replicas] minReplicas must be =< maxReplicas but wasn't 2 > 1", ex.getMessage()); } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java index 87860b8c536ef..978602bdf0cd5 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java @@ -44,6 +44,7 @@ import org.opensearch.common.io.stream.NamedWriteableRegistry; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.set.Sets; @@ -347,18 +348,15 @@ public void testNumberOfRoutingShards() { assertEquals(numShards, IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(build).intValue()); Settings lessThanSettings = Settings.builder().put("index.number_of_shards", 8).put("index.number_of_routing_shards", 4).build(); - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + SettingsException iae = expectThrows( + SettingsException.class, () -> IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(lessThanSettings) ); - assertEquals("index.number_of_routing_shards [4] must be >= index.number_of_shards [8]", iae.getMessage()); + assertEquals("index.number_of_routing_shards [4] must be >= index.number_of_shards [8]", iae.getCause().getMessage()); Settings notAFactorySettings = Settings.builder().put("index.number_of_shards", 2).put("index.number_of_routing_shards", 3).build(); - iae = expectThrows( - IllegalArgumentException.class, - () -> IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(notAFactorySettings) - ); - assertEquals("the number of source shards [2] must be a factor of [3]", iae.getMessage()); + iae = expectThrows(SettingsException.class, () -> IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(notAFactorySettings)); + assertEquals("the number of source shards [2] must be a factor of [3]", iae.getCause().getMessage()); } public void testMissingNumberOfShards() { @@ -376,12 +374,9 @@ public void testNumberOfShardsIsNotNegative() { private void runTestNumberOfShardsIsPositive(final int numberOfShards) { final Settings settings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numberOfShards).build(); - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> IndexMetadata.builder("test").settings(settings).build() - ); + final SettingsException e = expectThrows(SettingsException.class, () -> IndexMetadata.builder("test").settings(settings).build()); assertThat( - e.getMessage(), + e.getCause().getMessage(), equalTo("Failed to parse value [" + numberOfShards + "] for setting [index.number_of_shards] must be >= 1") ); } @@ -401,12 +396,9 @@ public void testNumberOfReplicasIsNonNegative() { .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, randomIntBetween(1, 8)) .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numberOfReplicas) .build(); - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> IndexMetadata.builder("test").settings(settings).build() - ); + final SettingsException e = expectThrows(SettingsException.class, () -> IndexMetadata.builder("test").settings(settings).build()); assertThat( - e.getMessage(), + e.getCause().getMessage(), equalTo("Failed to parse value [" + numberOfReplicas + "] for setting [index.number_of_replicas] must be >= 0") ); } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index bdf4d4d09ee6f..eaad61c7ad7be 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -378,8 +378,8 @@ public void testAddComponentTemplate() throws Exception { assertThat(state.metadata().componentTemplates().get("foo"), equalTo(componentTemplate)); final ClusterState throwState = ClusterState.builder(state).build(); - SettingsException e = expectThrows( - SettingsException.class, + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, () -> metadataIndexTemplateService.addComponentTemplate(throwState, true, "foo", componentTemplate) ); assertThat(e.getMessage(), containsString("component template [foo] already exists")); diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java index 5184ca7fe887d..97a379e03ba92 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java @@ -34,6 +34,7 @@ import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.test.OpenSearchTestCase; @@ -221,9 +222,8 @@ public void testInvalidHighDiskThreshold() { final String expected = "illegal value can't update [cluster.routing.allocation.disk.watermark.high] from [90%] to [75%]"; assertThat(e, hasToString(containsString(expected))); assertNotNull(e.getCause()); - assertNotNull(e.getCause()); - assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); - final IllegalArgumentException cause = (IllegalArgumentException) e.getCause(); + assertThat(e.getCause(), instanceOf(SettingsException.class)); + final SettingsException cause = (SettingsException) e.getCause(); assertThat(cause, hasToString(containsString("low disk watermark [85%] more than high disk watermark [75%]"))); } diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index 1f30bd8895bd9..c91989b24ae6a 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -1104,11 +1104,11 @@ public void testLoggingUpdates() { Settings.Builder builder = Settings.builder().put("logger.level", property); try { ClusterSettings settings = new ClusterSettings(builder.build(), ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); - IllegalArgumentException ex = expectThrows( - IllegalArgumentException.class, + SettingsException ex = expectThrows( + SettingsException.class, () -> settings.validate(Settings.builder().put("logger._root", "boom").build(), false) ); - assertEquals("Unknown level constant [BOOM].", ex.getMessage()); + assertEquals("Unknown level constant [BOOM].", ex.getCause().getMessage()); assertEquals(level, LogManager.getRootLogger().getLevel()); settings.applySettings(Settings.builder().put("logger._root", "TRACE").build()); assertEquals(Level.TRACE, LogManager.getRootLogger().getLevel()); From 701bd17acd055807f4855b932f075929e112ac69 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 9 Nov 2022 20:06:40 -0800 Subject: [PATCH 12/19] fix test failure Signed-off-by: Xue Zhou --- .../discovery/azure/classic/AzureSimpleTests.java | 7 ++----- .../customsettings/ExampleCustomSettingsConfigTests.java | 9 +++------ 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java b/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java index 95117beb3a0a9..4c2dda180eda6 100644 --- a/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java +++ b/plugins/discovery-azure-classic/src/internalClusterTest/java/org/opensearch/discovery/azure/classic/AzureSimpleTests.java @@ -36,11 +36,9 @@ import org.opensearch.cloud.azure.classic.management.AzureComputeService.Discovery; import org.opensearch.cloud.azure.classic.management.AzureComputeService.Management; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchIntegTestCase; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.instanceOf; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, numClientNodes = 0) public class AzureSimpleTests extends AbstractAzureComputeServiceTestCase { @@ -80,8 +78,7 @@ public void testOneNodeShouldRunUsingWrongSettings() { .put(Management.SERVICE_NAME_SETTING.getKey(), "dummy") .put(Discovery.HOST_TYPE_SETTING.getKey(), "do_not_exist"); - SettingsException e = expectThrows(SettingsException.class, () -> internalCluster().startNode(settings)); - assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); - assertThat(e.getCause().getMessage(), containsString("invalid value for host type [do_not_exist]")); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNode(settings)); + assertThat(e.getMessage(), containsString("invalid value for host type [do_not_exist]")); } } diff --git a/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java b/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java index 100edcf0eb79f..eb1fc056beaff 100644 --- a/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java +++ b/plugins/examples/custom-settings/src/test/java/org/opensearch/example/customsettings/ExampleCustomSettingsConfigTests.java @@ -31,9 +31,7 @@ package org.opensearch.example.customsettings; -import org.hamcrest.Matchers; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import static org.opensearch.example.customsettings.ExampleCustomSettingsConfig.VALIDATED_SETTING; @@ -52,11 +50,10 @@ public void testValidatedSetting() { final String actual = VALIDATED_SETTING.get(Settings.builder().put(VALIDATED_SETTING.getKey(), expected).build()); assertEquals(expected, actual); - final SettingsException exception = expectThrows( - SettingsException.class, + final IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, () -> VALIDATED_SETTING.get(Settings.builder().put("custom.validated", "it's forbidden").build()) ); - assertThat(exception.getCause(), Matchers.instanceOf(IllegalArgumentException.class)); - assertEquals("Setting must not contain [forbidden]", exception.getCause().getMessage()); + assertEquals("Setting must not contain [forbidden]", exception.getMessage()); } } From 4ec26f12230d4edd5882b7374f99b41a98423e81 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 9 Nov 2022 21:20:11 -0800 Subject: [PATCH 13/19] fix test failure Signed-off-by: Xue Zhou --- ...eCloudStorageBlobStoreRepositoryTests.java | 13 +++---- .../settings/SettingsUpdaterTests.java | 7 ++-- .../action/support/AutoCreateIndexTests.java | 9 ++--- .../ElectionSchedulerFactoryTests.java | 37 ++++++------------- .../NoClusterManagerBlockServiceTests.java | 3 +- .../metadata/AutoExpandReplicasTests.java | 11 +++--- .../cluster/metadata/IndexMetadataTests.java | 28 +++++++++----- .../DiskThresholdSettingsTests.java | 5 +-- .../common/settings/ScopedSettingsTests.java | 10 ++--- 9 files changed, 56 insertions(+), 67 deletions(-) diff --git a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java index c43470871e76a..c0a396724897f 100644 --- a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java +++ b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java @@ -55,7 +55,6 @@ import org.opensearch.common.regex.Regex; import org.opensearch.common.settings.MockSecureSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -164,7 +163,7 @@ public void testChunkSize() { assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), chunkSize); // zero bytes is not allowed - SettingsException e = expectThrows(SettingsException.class, () -> { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { final RepositoryMetadata repoMetadata = new RepositoryMetadata( "repo", GoogleCloudStorageRepository.TYPE, @@ -172,10 +171,10 @@ public void testChunkSize() { ); GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetadata); }); - assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); + assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getMessage()); // negative bytes not allowed - e = expectThrows(SettingsException.class, () -> { + e = expectThrows(IllegalArgumentException.class, () -> { final RepositoryMetadata repoMetadata = new RepositoryMetadata( "repo", GoogleCloudStorageRepository.TYPE, @@ -183,10 +182,10 @@ public void testChunkSize() { ); GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetadata); }); - assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getCause().getMessage()); + assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getMessage()); // greater than max chunk size not allowed - e = expectThrows(SettingsException.class, () -> { + e = expectThrows(IllegalArgumentException.class, () -> { final RepositoryMetadata repoMetadata = new RepositoryMetadata( "repo", GoogleCloudStorageRepository.TYPE, @@ -194,7 +193,7 @@ public void testChunkSize() { ); GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetadata); }); - assertEquals("failed to parse value [6tb] for setting [chunk_size], must be <= [5tb]", e.getCause().getMessage()); + assertEquals("failed to parse value [6tb] for setting [chunk_size], must be <= [5tb]", e.getMessage()); } public void testWriteReadLarge() throws IOException { diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 48b50813929f5..8d65c24f36b63 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -39,7 +39,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; @@ -158,7 +157,7 @@ public void testAllOrNothing() { logger ); fail("all or nothing"); - } catch (SettingsException ex) { + } catch (IllegalArgumentException ex) { logger.info("", ex); assertEquals("Failed to parse value [not a float] for setting [cluster.routing.allocation.balance.index]", ex.getMessage()); } @@ -664,11 +663,11 @@ public void testUpdateOfValidationDependentSettings() { final ClusterState finalCluster = cluster; Exception exception = expectThrows( - SettingsException.class, + IllegalArgumentException.class, () -> updater.updateSettings(finalCluster, Settings.builder().put(SETTING_FOO_HIGH.getKey(), 2).build(), Settings.EMPTY, logger) ); - assertThat(exception.getCause().getMessage(), equalTo("[high]=2 is lower than [low]=5")); + assertThat(exception.getMessage(), equalTo("[high]=2 is lower than [low]=5")); } } diff --git a/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java b/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java index 818c3c645f46d..068414678e860 100644 --- a/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java +++ b/server/src/test/java/org/opensearch/action/support/AutoCreateIndexTests.java @@ -41,7 +41,6 @@ import org.opensearch.common.collect.Tuple; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.index.IndexNotFoundException; import org.opensearch.indices.SystemIndexDescriptor; @@ -63,11 +62,11 @@ public void testParseFailed() { Settings settings = Settings.builder().put("action.auto_create_index", ",,,").build(); newAutoCreateIndex(settings); fail("initialization should have failed"); - } catch (SettingsException ex) { + } catch (IllegalArgumentException ex) { assertEquals( "Can't parse [,,,] for setting [action.auto_create_index] must be either [true, false, or a " + "comma separated list of index patterns]", - ex.getCause().getMessage() + ex.getMessage() ); } } @@ -78,10 +77,10 @@ public void testParseFailedMissingIndex() { try { newAutoCreateIndex(settings); fail("initialization should have failed"); - } catch (SettingsException ex) { + } catch (IllegalArgumentException ex) { assertEquals( "Can't parse [" + prefix + "] for setting [action.auto_create_index] must contain an index name after [" + prefix + "]", - ex.getCause().getMessage() + ex.getMessage() ); } } diff --git a/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java b/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java index 022a665b24bfc..164f802fdae38 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/ElectionSchedulerFactoryTests.java @@ -35,7 +35,6 @@ import org.opensearch.common.lease.Releasable; import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.Settings.Builder; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.test.OpenSearchTestCase; @@ -181,56 +180,44 @@ public void testRetriesOnCorrectSchedule() { public void testSettingsValidation() { { final Settings settings = Settings.builder().put(ELECTION_INITIAL_TIMEOUT_SETTING.getKey(), "0s").build(); - SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); - assertThat( - e.getCause().getMessage(), - is("failed to parse value [0s] for setting [cluster.election.initial_timeout], must be >= [1ms]") - ); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); + assertThat(e.getMessage(), is("failed to parse value [0s] for setting [cluster.election.initial_timeout], must be >= [1ms]")); } { final Settings settings = Settings.builder().put(ELECTION_INITIAL_TIMEOUT_SETTING.getKey(), "10001ms").build(); - SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_INITIAL_TIMEOUT_SETTING.get(settings)); assertThat( - e.getCause().getMessage(), + e.getMessage(), is("failed to parse value [10001ms] for setting [cluster.election.initial_timeout], must be <= [10s]") ); } { final Settings settings = Settings.builder().put(ELECTION_BACK_OFF_TIME_SETTING.getKey(), "0s").build(); - SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); - assertThat( - e.getCause().getMessage(), - is("failed to parse value [0s] for setting [cluster.election.back_off_time], must be >= [1ms]") - ); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); + assertThat(e.getMessage(), is("failed to parse value [0s] for setting [cluster.election.back_off_time], must be >= [1ms]")); } { final Settings settings = Settings.builder().put(ELECTION_BACK_OFF_TIME_SETTING.getKey(), "60001ms").build(); - SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_BACK_OFF_TIME_SETTING.get(settings)); assertThat( - e.getCause().getMessage(), + e.getMessage(), is("failed to parse value [60001ms] for setting [cluster.election.back_off_time], must be <= [60s]") ); } { final Settings settings = Settings.builder().put(ELECTION_MAX_TIMEOUT_SETTING.getKey(), "199ms").build(); - SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); - assertThat( - e.getCause().getMessage(), - is("failed to parse value [199ms] for setting [cluster.election.max_timeout], must be >= [200ms]") - ); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); + assertThat(e.getMessage(), is("failed to parse value [199ms] for setting [cluster.election.max_timeout], must be >= [200ms]")); } { final Settings settings = Settings.builder().put(ELECTION_MAX_TIMEOUT_SETTING.getKey(), "301s").build(); - SettingsException e = expectThrows(SettingsException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); - assertThat( - e.getCause().getMessage(), - is("failed to parse value [301s] for setting [cluster.election.max_timeout], must be <= [300s]") - ); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ELECTION_MAX_TIMEOUT_SETTING.get(settings)); + assertThat(e.getMessage(), is("failed to parse value [301s] for setting [cluster.election.max_timeout], must be <= [300s]")); } { diff --git a/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java b/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java index 515d868a60206..7bd2fefe50c42 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/NoClusterManagerBlockServiceTests.java @@ -33,7 +33,6 @@ import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.test.OpenSearchTestCase; import static org.opensearch.cluster.coordination.NoClusterManagerBlockService.NO_CLUSTER_MANAGER_BLOCK_ALL; @@ -75,7 +74,7 @@ public void testBlocksMetadataWritesIfConfiguredBySetting() { public void testRejectsInvalidSetting() { expectThrows( - SettingsException.class, + IllegalArgumentException.class, () -> createService(Settings.builder().put(NO_CLUSTER_MANAGER_BLOCK_SETTING.getKey(), "unknown").build()) ); } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java b/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java index 787853262c8d7..4b7eaf0272a91 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/AutoExpandReplicasTests.java @@ -44,7 +44,6 @@ import org.opensearch.cluster.routing.IndexShardRoutingTable; import org.opensearch.cluster.routing.ShardRoutingState; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.indices.cluster.ClusterStateChanges; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; @@ -93,29 +92,29 @@ public void testInvalidValues() { try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "boom").build()); fail(); - } catch (SettingsException ex) { - assertEquals("failed to parse [index.auto_expand_replicas] from value: [boom] at index -1", ex.getCause().getMessage()); + } catch (IllegalArgumentException ex) { + assertEquals("failed to parse [index.auto_expand_replicas] from value: [boom] at index -1", ex.getMessage()); } try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "1-boom").build()); fail(); } catch (IllegalArgumentException ex) { - assertEquals("failed to parse [index.auto_expand_replicas] from value: [1-boom] at index 1", ex.getCause().getMessage()); + assertEquals("failed to parse [index.auto_expand_replicas] from value: [1-boom] at index 1", ex.getMessage()); assertEquals("For input string: \"boom\"", ex.getCause().getMessage()); } try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "boom-1").build()); fail(); - } catch (SettingsException ex) { + } catch (IllegalArgumentException ex) { assertEquals("failed to parse [index.auto_expand_replicas] from value: [boom-1] at index 4", ex.getMessage()); assertEquals("For input string: \"boom\"", ex.getCause().getMessage()); } try { AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "2-1").build()); - } catch (SettingsException ex) { + } catch (IllegalArgumentException ex) { assertEquals("[index.auto_expand_replicas] minReplicas must be =< maxReplicas but wasn't 2 > 1", ex.getMessage()); } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java index 978602bdf0cd5..87860b8c536ef 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java @@ -44,7 +44,6 @@ import org.opensearch.common.io.stream.NamedWriteableRegistry; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.set.Sets; @@ -348,15 +347,18 @@ public void testNumberOfRoutingShards() { assertEquals(numShards, IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(build).intValue()); Settings lessThanSettings = Settings.builder().put("index.number_of_shards", 8).put("index.number_of_routing_shards", 4).build(); - SettingsException iae = expectThrows( - SettingsException.class, + IllegalArgumentException iae = expectThrows( + IllegalArgumentException.class, () -> IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(lessThanSettings) ); - assertEquals("index.number_of_routing_shards [4] must be >= index.number_of_shards [8]", iae.getCause().getMessage()); + assertEquals("index.number_of_routing_shards [4] must be >= index.number_of_shards [8]", iae.getMessage()); Settings notAFactorySettings = Settings.builder().put("index.number_of_shards", 2).put("index.number_of_routing_shards", 3).build(); - iae = expectThrows(SettingsException.class, () -> IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(notAFactorySettings)); - assertEquals("the number of source shards [2] must be a factor of [3]", iae.getCause().getMessage()); + iae = expectThrows( + IllegalArgumentException.class, + () -> IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.get(notAFactorySettings) + ); + assertEquals("the number of source shards [2] must be a factor of [3]", iae.getMessage()); } public void testMissingNumberOfShards() { @@ -374,9 +376,12 @@ public void testNumberOfShardsIsNotNegative() { private void runTestNumberOfShardsIsPositive(final int numberOfShards) { final Settings settings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numberOfShards).build(); - final SettingsException e = expectThrows(SettingsException.class, () -> IndexMetadata.builder("test").settings(settings).build()); + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> IndexMetadata.builder("test").settings(settings).build() + ); assertThat( - e.getCause().getMessage(), + e.getMessage(), equalTo("Failed to parse value [" + numberOfShards + "] for setting [index.number_of_shards] must be >= 1") ); } @@ -396,9 +401,12 @@ public void testNumberOfReplicasIsNonNegative() { .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, randomIntBetween(1, 8)) .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numberOfReplicas) .build(); - final SettingsException e = expectThrows(SettingsException.class, () -> IndexMetadata.builder("test").settings(settings).build()); + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> IndexMetadata.builder("test").settings(settings).build() + ); assertThat( - e.getCause().getMessage(), + e.getMessage(), equalTo("Failed to parse value [" + numberOfReplicas + "] for setting [index.number_of_replicas] must be >= 0") ); } diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java index 97a379e03ba92..d23b079e35ef9 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java @@ -34,7 +34,6 @@ import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeValue; import org.opensearch.test.OpenSearchTestCase; @@ -222,8 +221,8 @@ public void testInvalidHighDiskThreshold() { final String expected = "illegal value can't update [cluster.routing.allocation.disk.watermark.high] from [90%] to [75%]"; assertThat(e, hasToString(containsString(expected))); assertNotNull(e.getCause()); - assertThat(e.getCause(), instanceOf(SettingsException.class)); - final SettingsException cause = (SettingsException) e.getCause(); + assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); + final IllegalArgumentException cause = (IllegalArgumentException) e.getCause(); assertThat(cause, hasToString(containsString("low disk watermark [85%] more than high disk watermark [75%]"))); } diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index c91989b24ae6a..efeb7b3c29d3a 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -1057,14 +1057,14 @@ public void testKeyPattern() { try { new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.groupSetting("foo.bar.", Property.IndexScope))); fail(); - } catch (SettingsException e) { + } catch (IllegalArgumentException e) { assertEquals("illegal settings key: [foo.bar.] must start with [index.]", e.getMessage()); } try { new IndexScopedSettings(Settings.EMPTY, Collections.singleton(Setting.simpleString("foo.bar", Property.IndexScope))); fail(); - } catch (SettingsException e) { + } catch (IllegalArgumentException e) { assertEquals("illegal settings key: [foo.bar] must start with [index.]", e.getMessage()); } @@ -1104,11 +1104,11 @@ public void testLoggingUpdates() { Settings.Builder builder = Settings.builder().put("logger.level", property); try { ClusterSettings settings = new ClusterSettings(builder.build(), ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); - SettingsException ex = expectThrows( - SettingsException.class, + IllegalArgumentException ex = expectThrows( + IllegalArgumentException.class, () -> settings.validate(Settings.builder().put("logger._root", "boom").build(), false) ); - assertEquals("Unknown level constant [BOOM].", ex.getCause().getMessage()); + assertEquals("Unknown level constant [BOOM].", ex.getMessage()); assertEquals(level, LogManager.getRootLogger().getLevel()); settings.applySettings(Settings.builder().put("logger._root", "TRACE").build()); assertEquals(Level.TRACE, LogManager.getRootLogger().getLevel()); From 39a4b995d3d61f1c30a47445ab14962882093016 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 9 Nov 2022 22:31:37 -0800 Subject: [PATCH 14/19] fix test failure Signed-off-by: Xue Zhou --- .../common/settings/ScopedSettingsTests.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index efeb7b3c29d3a..da05eb251d643 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -976,26 +976,26 @@ public void testValidate() { + " removed settings"; settings.validate(Settings.builder().put("index.store.type", "boom").build(), false); - SettingsException e = expectThrows( + SettingsException settingsException = expectThrows( SettingsException.class, () -> settings.validate(Settings.builder().put("index.store.type", "boom").put("i.am.not.a.setting", true).build(), false) ); - assertEquals("unknown setting [i.am.not.a.setting]" + unknownMsgSuffix, e.getMessage()); + assertEquals("unknown setting [i.am.not.a.setting]" + unknownMsgSuffix, settingsException.getMessage()); - e = expectThrows( - SettingsException.class, + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, () -> settings.validate(Settings.builder().put("index.store.type", "boom").put("index.number_of_replicas", true).build(), false) ); assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage()); e = expectThrows( - SettingsException.class, + IllegalArgumentException.class, () -> settings.validate("index.number_of_replicas", Settings.builder().put("index.number_of_replicas", "true").build(), false) ); assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage()); e = expectThrows( - SettingsException.class, + IllegalArgumentException.class, () -> settings.validate( "index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build(), From 1d3b642dc66a37c27e39062091bdff682ab81388 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Thu, 10 Nov 2022 00:39:54 -0800 Subject: [PATCH 15/19] fix test failure Signed-off-by: Xue Zhou --- .../admin/indices/create/CreateIndexIT.java | 5 ++-- .../cluster/settings/ClusterSettingsIT.java | 3 ++- .../indices/settings/InternalSettingsIT.java | 5 ++-- .../indices/settings/PrivateSettingsIT.java | 5 ++-- .../indices/settings/UpdateSettingsIT.java | 25 ++++++++++--------- .../template/SimpleIndexTemplateIT.java | 5 ++-- 6 files changed, 27 insertions(+), 21 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/CreateIndexIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/CreateIndexIT.java index 51ef63ae9e9c1..39c2b5b113a43 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/CreateIndexIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/CreateIndexIT.java @@ -47,6 +47,7 @@ import org.opensearch.cluster.metadata.Metadata; import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.index.IndexNotFoundException; @@ -82,7 +83,7 @@ public void testCreationDateGivenFails() { try { prepareCreate("test").setSettings(Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 4L)).get(); fail(); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals( "unknown setting [index.creation_date] please check that any required plugins are installed, or check the " + "breaking changes documentation for removed settings", @@ -203,7 +204,7 @@ public void testUnknownSettingFails() { try { prepareCreate("test").setSettings(Settings.builder().put("index.unknown.value", "this must fail").build()).get(); fail("should have thrown an exception about the shard count"); - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { assertEquals( "unknown setting [index.unknown.value] please check that any required plugins are installed, or check the" + " breaking changes documentation for removed settings", diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/settings/ClusterSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/settings/ClusterSettingsIT.java index 14365bba1e016..79b674b23fd48 100644 --- a/server/src/internalClusterTest/java/org/opensearch/cluster/settings/ClusterSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/settings/ClusterSettingsIT.java @@ -41,6 +41,7 @@ import org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.TimeValue; import org.opensearch.indices.recovery.RecoverySettings; @@ -78,7 +79,7 @@ public void testClusterNonExistingSettingsUpdate() { try { client().admin().cluster().prepareUpdateSettings().setTransientSettings(Settings.builder().put(key1, value1).build()).get(); fail("bogus value"); - } catch (IllegalArgumentException ex) { + } catch (SettingsException ex) { assertEquals("transient setting [no_idea_what_you_are_talking_about], not recognized", ex.getMessage()); } } diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/InternalSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/InternalSettingsIT.java index c8e2a45891cb3..6a05e893d4fc9 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/InternalSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/InternalSettingsIT.java @@ -34,6 +34,7 @@ import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.plugins.Plugin; import org.opensearch.test.OpenSearchIntegTestCase; @@ -64,8 +65,8 @@ public void testUpdateInternalIndexSettingViaSettingsAPI() { final GetSettingsResponse response = client().admin().indices().prepareGetSettings("test").get(); assertThat(response.getSetting("test", "index.internal"), equalTo("internal")); // we can not update the setting via the update settings API - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> client().admin() .indices() .prepareUpdateSettings("test") diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/PrivateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/PrivateSettingsIT.java index c089381478dd3..0ff41f37d4000 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/PrivateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/PrivateSettingsIT.java @@ -35,6 +35,7 @@ import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse; import org.opensearch.common.ValidationException; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.plugins.Plugin; import org.opensearch.test.OpenSearchIntegTestCase; @@ -64,8 +65,8 @@ public void testSetPrivateIndexSettingOnCreate() { public void testUpdatePrivateIndexSettingViaSettingsAPI() { createIndex("test"); // we can not update the setting via the update settings API - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + final SettingsException e = expectThrows( + SettingsException.class, () -> client().admin() .indices() .prepareUpdateSettings("test") diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java index 9970ff99a806c..73dcb2de645e6 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java @@ -40,6 +40,7 @@ import org.opensearch.common.Priority; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.TimeValue; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexService; @@ -174,8 +175,8 @@ protected Settings nodeSettings(int nodeOrdinal) { } public void testUpdateDependentClusterSettings() { - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + SettingsException iae = expectThrows( + SettingsException.class, () -> client().admin() .cluster() .prepareUpdateSettings() @@ -185,7 +186,7 @@ public void testUpdateDependentClusterSettings() { assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); iae = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> client().admin() .cluster() .prepareUpdateSettings() @@ -195,7 +196,7 @@ public void testUpdateDependentClusterSettings() { assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); iae = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> client().admin() .cluster() .prepareUpdateSettings() @@ -212,7 +213,7 @@ public void testUpdateDependentClusterSettings() { .setTransientSettings(Settings.builder().put("cluster.acc.test.pw", "asdf").put("cluster.acc.test.user", "asdf")) .get(); iae = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> client().admin() .cluster() .prepareUpdateSettings() @@ -233,7 +234,7 @@ public void testUpdateDependentClusterSettings() { .get(); iae = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> client().admin() .cluster() .prepareUpdateSettings() @@ -252,8 +253,8 @@ public void testUpdateDependentClusterSettings() { } public void testUpdateDependentIndexSettings() { - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + SettingsException iae = expectThrows( + SettingsException.class, () -> prepareCreate("test", Settings.builder().put("index.acc.test.pw", "asdf")).get() ); assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage()); @@ -266,7 +267,7 @@ public void testUpdateDependentIndexSettings() { } iae = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> client().admin() .indices() .prepareUpdateSettings("test") @@ -294,7 +295,7 @@ public void testUpdateDependentIndexSettings() { // now try to remove it and make sure it fails iae = expectThrows( - IllegalArgumentException.class, + SettingsException.class, () -> client().admin() .indices() .prepareUpdateSettings("test") @@ -480,8 +481,8 @@ public void testOpenCloseUpdateSettings() throws Exception { assertThat(indexMetadata.getSettings().get("index.refresh_interval"), equalTo("1s")); assertThat(indexMetadata.getSettings().get("index.fielddata.cache"), equalTo("none")); - IllegalArgumentException ex = expectThrows( - IllegalArgumentException.class, + SettingsException ex = expectThrows( + SettingsException.class, () -> client().admin() .indices() .prepareUpdateSettings("test") diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java index c97b8e932effc..5ab32681fd79b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/template/SimpleIndexTemplateIT.java @@ -48,6 +48,7 @@ import org.opensearch.common.ParsingException; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; import org.opensearch.index.mapper.MapperParsingException; @@ -494,8 +495,8 @@ public void testInvalidSettings() throws Exception { GetIndexTemplatesResponse response = client().admin().indices().prepareGetTemplates().get(); assertThat(response.getIndexTemplates(), empty()); - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + SettingsException e = expectThrows( + SettingsException.class, () -> client().admin() .indices() .preparePutTemplate("template_1") From 22adbd3d9a7cc6ed2d43dd2e928c42f8c8cbd450 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Tue, 10 Jan 2023 22:42:26 -0800 Subject: [PATCH 16/19] Modify comments Signed-off-by: Xue Zhou --- CHANGELOG.md | 1 + .../indices/settings/UpdateSettingsIT.java | 32 +++++++++---------- .../common/settings/ScopedSettingsTests.java | 12 +++---- .../opensearch/search/SearchServiceTests.java | 4 +-- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1713e18781ded..7487a24ea0bf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283)) - Pre conditions check before updating weighted routing metadata ([#4955](https://github.com/opensearch-project/OpenSearch/pull/4955)) - Gracefully handle concurrent zone decommission action ([#5542](https://github.com/opensearch-project/OpenSearch/pull/5542)) +- Mapping IllegalArgumentException to SettingsException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) ### Deprecated diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java index 73dcb2de645e6..af1fdfed6512a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java @@ -175,7 +175,7 @@ protected Settings nodeSettings(int nodeOrdinal) { } public void testUpdateDependentClusterSettings() { - SettingsException iae = expectThrows( + SettingsException e = expectThrows( SettingsException.class, () -> client().admin() .cluster() @@ -183,9 +183,9 @@ public void testUpdateDependentClusterSettings() { .setPersistentSettings(Settings.builder().put("cluster.acc.test.pw", "asdf")) .get() ); - assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage()); - iae = expectThrows( + e = expectThrows( SettingsException.class, () -> client().admin() .cluster() @@ -193,9 +193,9 @@ public void testUpdateDependentClusterSettings() { .setTransientSettings(Settings.builder().put("cluster.acc.test.pw", "asdf")) .get() ); - assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage()); - iae = expectThrows( + e = expectThrows( SettingsException.class, () -> client().admin() .cluster() @@ -204,7 +204,7 @@ public void testUpdateDependentClusterSettings() { .setPersistentSettings(Settings.builder().put("cluster.acc.test.user", "asdf")) .get() ); - assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage()); if (randomBoolean()) { client().admin() @@ -212,7 +212,7 @@ public void testUpdateDependentClusterSettings() { .prepareUpdateSettings() .setTransientSettings(Settings.builder().put("cluster.acc.test.pw", "asdf").put("cluster.acc.test.user", "asdf")) .get(); - iae = expectThrows( + e = expectThrows( SettingsException.class, () -> client().admin() .cluster() @@ -220,7 +220,7 @@ public void testUpdateDependentClusterSettings() { .setTransientSettings(Settings.builder().putNull("cluster.acc.test.user")) .get() ); - assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage()); client().admin() .cluster() .prepareUpdateSettings() @@ -233,7 +233,7 @@ public void testUpdateDependentClusterSettings() { .setPersistentSettings(Settings.builder().put("cluster.acc.test.pw", "asdf").put("cluster.acc.test.user", "asdf")) .get(); - iae = expectThrows( + e = expectThrows( SettingsException.class, () -> client().admin() .cluster() @@ -241,7 +241,7 @@ public void testUpdateDependentClusterSettings() { .setPersistentSettings(Settings.builder().putNull("cluster.acc.test.user")) .get() ); - assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", e.getMessage()); client().admin() .cluster() @@ -253,11 +253,11 @@ public void testUpdateDependentClusterSettings() { } public void testUpdateDependentIndexSettings() { - SettingsException iae = expectThrows( + SettingsException e = expectThrows( SettingsException.class, () -> prepareCreate("test", Settings.builder().put("index.acc.test.pw", "asdf")).get() ); - assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", e.getMessage()); createIndex("test"); for (int i = 0; i < 2; i++) { @@ -266,7 +266,7 @@ public void testUpdateDependentIndexSettings() { client().admin().indices().prepareClose("test").get(); } - iae = expectThrows( + e = expectThrows( SettingsException.class, () -> client().admin() .indices() @@ -275,7 +275,7 @@ public void testUpdateDependentIndexSettings() { .execute() .actionGet() ); - assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", e.getMessage()); // user has no dependency client().admin() @@ -294,7 +294,7 @@ public void testUpdateDependentIndexSettings() { .actionGet(); // now try to remove it and make sure it fails - iae = expectThrows( + e = expectThrows( SettingsException.class, () -> client().admin() .indices() @@ -303,7 +303,7 @@ public void testUpdateDependentIndexSettings() { .execute() .actionGet() ); - assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", iae.getMessage()); + assertEquals("missing required setting [index.acc.test.user] for setting [index.acc.test.pw]", e.getMessage()); // now we are consistent client().admin() diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index da05eb251d643..1dcb8ee00ebb2 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -208,11 +208,11 @@ public void testDependentSettings() { AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(intSetting, stringSetting))); - SettingsException iae = expectThrows( + SettingsException e = expectThrows( SettingsException.class, () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true) ); - assertEquals("missing required setting [foo.test.name] for setting [foo.test.bar]", iae.getMessage()); + assertEquals("missing required setting [foo.test.name] for setting [foo.test.bar]", e.getMessage()); service.validate(Settings.builder().put("foo.test.name", "test").put("foo.test.bar", 7).build(), true); @@ -247,11 +247,11 @@ public void validate(final String key, final Object value, final Object dependen AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(intSetting, stringSetting))); - SettingsException iae = expectThrows( + SettingsException e = expectThrows( SettingsException.class, () -> service.validate(Settings.builder().put("foo.test.bar", 7).put("foo.test.name", "invalid").build(), true) ); - assertEquals("[foo.test.bar] is set but [name] is [invalid]", iae.getMessage()); + assertEquals("[foo.test.bar] is set but [name] is [invalid]", e.getMessage()); service.validate(Settings.builder().put("foo.test.bar", 7).put("foo.test.name", "valid").build(), true); @@ -963,11 +963,11 @@ public void testGetSetting() { public void testValidateWithSuggestion() { IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); - SettingsException iae = expectThrows( + SettingsException e = expectThrows( SettingsException.class, () -> settings.validate(Settings.builder().put("index.numbe_of_replica", "1").build(), false) ); - assertEquals(iae.getMessage(), "unknown setting [index.numbe_of_replica] did you mean [index.number_of_replicas]?"); + assertEquals(e.getMessage(), "unknown setting [index.numbe_of_replica] did you mean [index.number_of_replicas]?"); } public void testValidate() { diff --git a/server/src/test/java/org/opensearch/search/SearchServiceTests.java b/server/src/test/java/org/opensearch/search/SearchServiceTests.java index 414c6b4bcb806..535eb3a118189 100644 --- a/server/src/test/java/org/opensearch/search/SearchServiceTests.java +++ b/server/src/test/java/org/opensearch/search/SearchServiceTests.java @@ -1072,7 +1072,7 @@ public void testSetSearchThrottled() { ) ).actionGet(); - SettingsException iae = expectThrows( + SettingsException se = expectThrows( SettingsException.class, () -> client().admin() .indices() @@ -1080,7 +1080,7 @@ public void testSetSearchThrottled() { .setSettings(Settings.builder().put(IndexSettings.INDEX_SEARCH_THROTTLED.getKey(), false)) .get() ); - assertEquals("can not update private setting [index.search.throttled]; this setting is managed by OpenSearch", iae.getMessage()); + assertEquals("can not update private setting [index.search.throttled]; this setting is managed by OpenSearch", se.getMessage()); assertFalse(service.getIndicesService().indexServiceSafe(index).getIndexSettings().isSearchThrottled()); SearchRequest searchRequest = new SearchRequest().allowPartialSearchResults(false); ShardSearchRequest req = new ShardSearchRequest( From 07f1f7252da3ccca772f65d32b5090f46e47d729 Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Wed, 11 Jan 2023 10:46:54 -0800 Subject: [PATCH 17/19] Modify changelog Signed-off-by: Xue Zhou --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7487a24ea0bf9..52f2a7aadd5c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,7 +75,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283)) - Pre conditions check before updating weighted routing metadata ([#4955](https://github.com/opensearch-project/OpenSearch/pull/4955)) - Gracefully handle concurrent zone decommission action ([#5542](https://github.com/opensearch-project/OpenSearch/pull/5542)) -- Mapping IllegalArgumentException to SettingsException extend OpenSearchException in AbstractScopedSettings class ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) +- Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) ### Deprecated From a6932d6be84b0f0697bcae7ac5c8907b5722c46d Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Sun, 5 Feb 2023 21:03:52 -0800 Subject: [PATCH 18/19] Modify changelog Signed-off-by: Xue Zhou --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6406993a86a58..6662eac9a885e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Changed http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773)) - Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283)) - Require MediaType in Strings.toString API ([#6009](https://github.com/opensearch-project/OpenSearch/pull/6009)) +- Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) ### Deprecated From fb82354bd8eaede669876e22d5e57a7e84abe5ba Mon Sep 17 00:00:00 2001 From: Xue Zhou Date: Sun, 5 Feb 2023 22:34:20 -0800 Subject: [PATCH 19/19] fix test failure Signed-off-by: Xue Zhou --- .../extensions/AddSettingsUpdateConsumerRequestHandler.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java b/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java index cb05a4210b483..b1c8b655c4c6f 100644 --- a/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java +++ b/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java @@ -14,6 +14,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.SettingsException; import org.opensearch.common.settings.WriteableSetting; import org.opensearch.transport.TransportResponse; import org.opensearch.transport.TransportService; @@ -81,7 +82,7 @@ TransportResponse handleAddSettingsUpdateConsumerRequest(AddSettingsUpdateConsum ); }); } - } catch (IllegalArgumentException e) { + } catch (SettingsException e) { logger.error(e.toString()); status = false; }