Skip to content

Commit

Permalink
refactor exception flow to improve error logging
Browse files Browse the repository at this point in the history
Signed-off-by: bansvaru <bansvaru@amazon.com>
  • Loading branch information
linuxpi committed Jul 21, 2023
1 parent e59c99e commit 036c85d
Show file tree
Hide file tree
Showing 6 changed files with 115 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,17 @@ public void testRemoteStoreDisabledByUser() throws Exception {
IllegalArgumentException.class,
() -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
Locale.ROOT,
"Cannot override [%s] settings when [%s] is set to [true].",
SETTING_REMOTE_STORE_ENABLED,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testRemoteStoreEnabledByUserWithoutRemoteRepoAndSegmentReplicationIllegalArgumentException() throws Exception {
Expand Down Expand Up @@ -147,7 +157,17 @@ public void testRemoteStoreEnabledByUserWithoutRemoteRepoIllegalArgumentExceptio
IllegalArgumentException.class,
() -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
Locale.ROOT,
"Cannot override [%s] settings when [%s] is set to [true].",
SETTING_REMOTE_STORE_ENABLED,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testReplicationTypeDocumentByUser() throws Exception {
Expand Down Expand Up @@ -197,7 +217,7 @@ public void testRemoteStoreSegmentRepoWithoutRemoteEnabledAndSegmentReplicationI
);
}

public void testRemoteStoreEnabledByUserWithRemoteRepo() throws Exception {
public void testRemoteStoreEnabledByUserWithRemoteRepoIllegalArgumentException() throws Exception {
Settings settings = Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
Expand All @@ -210,7 +230,18 @@ public void testRemoteStoreEnabledByUserWithRemoteRepo() throws Exception {
IllegalArgumentException.class,
() -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
Locale.ROOT,
"Cannot override [%s][%s] settings when [%s] is set to [true].",
SETTING_REMOTE_STORE_ENABLED,
SETTING_REMOTE_SEGMENT_STORE_REPOSITORY,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testRemoteStoreOverrideOnlyTranslogRepoIllegalArgumentException() throws Exception {
Expand Down Expand Up @@ -249,7 +280,19 @@ public void testRemoteStoreOverrideTranslogRepoCorrectly() throws Exception {
IllegalArgumentException.class,
() -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
Locale.ROOT,
"Cannot override [%s][%s][%s] settings when [%s] is set to [true].",
SETTING_REMOTE_STORE_ENABLED,
SETTING_REMOTE_SEGMENT_STORE_REPOSITORY,
SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testRemoteStoreOverrideReplicationTypeIndexSettings() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ public Iterator<Setting<?>> settings() {
/**
* Used to specify remote store repository to use for this index.
*/
public static final Setting<String> INDEX_REMOTE_STORE_REPOSITORY_SETTING = Setting.simpleString(
public static final Setting<String> INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING = Setting.simpleString(
SETTING_REMOTE_SEGMENT_STORE_REPOSITORY,
new Setting.Validator<>() {

Expand All @@ -345,10 +345,12 @@ public void validate(final String value) {}
public void validate(final String value, final Map<Setting<?>, Object> settings) {
if (value == null || value.isEmpty()) {
throw new IllegalArgumentException(
"Setting " + INDEX_REMOTE_STORE_REPOSITORY_SETTING.getKey() + " should be provided with non-empty repository ID"
"Setting "
+ INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.getKey()
+ " should be provided with non-empty repository ID"
);
} else {
validateRemoteStoreSettingEnabled(settings, INDEX_REMOTE_STORE_REPOSITORY_SETTING);
validateRemoteStoreSettingEnabled(settings, INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REPLICATION_TYPE_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS;
Expand Down Expand Up @@ -949,27 +949,39 @@ private static void updateReplicationStrategy(Settings.Builder settingsBuilder,
* @param clusterSettings cluster level settings
*/
private static void updateRemoteStoreSettings(Settings.Builder settingsBuilder, Settings requestSettings, Settings clusterSettings) {
if (CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings) == true) {
// User should not be able to override remote store index settings
if (canCreateRemoteStoreIndex(requestSettings) == false) {
throw new IllegalArgumentException("Cannot override settings related to remote store.");
}
boolean isRemoteStoreClusterEnabled = CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings);
List<String> overriddenSettings = getRemoteStoreOverriddenSetting(requestSettings);
if (overriddenSettings.isEmpty() == false) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
"Cannot override [%s] settings when [%s] is set to [%s].",
String.join("][", overriddenSettings),
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey(),
isRemoteStoreClusterEnabled
)
);
}

if (isRemoteStoreClusterEnabled == true) {
settingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, true)
.put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, CLUSTER_REMOTE_STORE_REPOSITORY_SETTING.get(clusterSettings))
.put(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(clusterSettings));
} else {
// User should not be able to create remote indices
if (canCreateRemoteStoreIndex(requestSettings) == false) {
throw new IllegalArgumentException(String.format(Locale.ROOT, "Cannot create remote store indices where [%s] is not set", CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()));
}
}
}

private static boolean canCreateRemoteStoreIndex(Settings requestSettings) {
return INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == false
&& INDEX_REMOTE_STORE_REPOSITORY_SETTING.exists(requestSettings) == false
&& INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.exists(requestSettings) == false;
private static List<String> getRemoteStoreOverriddenSetting(Settings requestSettings) {
List<String> overriddenSettings = new ArrayList<>();
if (INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == true) {
overriddenSettings.add(SETTING_REMOTE_STORE_ENABLED);
}
if (INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.exists(requestSettings) == true) {
overriddenSettings.add(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY);
}
if (INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.exists(requestSettings) == true) {
overriddenSettings.add(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY);
}
return overriddenSettings;
}

public static void validateStoreTypeSettings(Settings settings) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
FeatureFlags.REMOTE_STORE,
List.of(
IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING,
IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING,
IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING,
IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING
),
FeatureFlags.CONCURRENT_SEGMENT_SEARCH,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,16 @@ public void testRemoteStoreDisabledByUserIndexSettings() {
Collections.emptySet()
)
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
"Cannot override [%s] settings when [%s] is set to [true].",
SETTING_REMOTE_STORE_ENABLED,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testRemoteStoreOverrideSegmentRepoIndexSettings() {
Expand Down Expand Up @@ -1311,7 +1320,17 @@ public void testRemoteStoreOverrideSegmentRepoIndexSettings() {
Collections.emptySet()
)
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
"Cannot override [%s][%s] settings when [%s] is set to [true].",
SETTING_REMOTE_STORE_ENABLED,
SETTING_REMOTE_SEGMENT_STORE_REPOSITORY,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testRemoteStoreOverrideTranslogRepoIndexSettings() {
Expand Down Expand Up @@ -1340,7 +1359,16 @@ public void testRemoteStoreOverrideTranslogRepoIndexSettings() {
Collections.emptySet()
)
);
assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
assertThat(
exc.getMessage(),
containsString(
String.format(
"Cannot override [%s] settings when [%s] is set to [true].",
SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY,
CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey()
)
)
);
}

public void testBuildIndexMetadata() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ public void testRemoteRepositoryExplicitSetting() {

public void testUpdateRemoteRepositoryFails() {
Set<Setting<?>> remoteStoreSettingSet = new HashSet<>();
remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING);
remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING);
IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, remoteStoreSettingSet);
SettingsException error = expectThrows(
SettingsException.class,
Expand All @@ -881,7 +881,7 @@ public void testSetRemoteRepositoryFailsWhenRemoteStoreIsNotEnabled() {
.build();
IllegalArgumentException iae = expectThrows(
IllegalArgumentException.class,
() -> IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING.get(indexSettings)
() -> IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.get(indexSettings)
);
assertEquals(
String.format(
Expand All @@ -902,7 +902,7 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() {
.build();
IllegalArgumentException iae = expectThrows(
IllegalArgumentException.class,
() -> IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING.get(indexSettings)
() -> IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.get(indexSettings)
);
assertEquals(
String.format(
Expand Down

0 comments on commit 036c85d

Please sign in to comment.