Skip to content

Commit

Permalink
Addressing comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Shourya Dutta Biswas <114977491+shourya035@users.noreply.github.com>
  • Loading branch information
@shourya035
shourya035 committed Apr 24, 2024
1 parent 7e1d9df commit 6f9f0c3
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 93 deletions.
15 changes: 6 additions & 9 deletions server/src/main/java/org/opensearch/cluster/routing/allocation/IndexMetadataUpdater.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,20 +177,17 @@ public Metadata applyChanges(Metadata oldMetadata, RoutingTable newRoutingTable,
indexMetadataBuilder = updateInSyncAllocations(newRoutingTable, oldIndexMetadata, indexMetadataBuilder, shardId, updates);
indexMetadataBuilder = updatePrimaryTerm(oldIndexMetadata, indexMetadataBuilder, shardId, updates);
if (ongoingRemoteStoreMigration) {
RemoteMigrationIndexMetadataUpdater migrationImdUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
migrationImdUpdater.maybeUpdateRemoteStorePathStrategy(
oldIndexMetadata,
indexMetadataBuilder,
index.getName(),
RemoteMigrationIndexMetadataUpdater migrationImdUpdater = new RemoteMigrationIndexMetadataUpdater(
discoveryNodes,
oldMetadata.settings()
newRoutingTable,
oldIndexMetadata,
oldMetadata.settings(),
logger
);
migrationImdUpdater.maybeUpdateRemoteStorePathStrategy(indexMetadataBuilder, index.getName());
migrationImdUpdater.maybeAddRemoteIndexSettings(
oldIndexMetadata,
indexMetadataBuilder,
newRoutingTable,
index.getName(),
discoveryNodes,
remoteRepoNames.get(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY),
remoteRepoNames.get(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY)
);
Expand Down
35 changes: 19 additions & 16 deletions server/src/main/java/org/opensearch/index/remote/RemoteMigrationIndexMetadataUpdater.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,24 @@
* @opensearch.internal
*/
public class RemoteMigrationIndexMetadataUpdater {
private final DiscoveryNodes discoveryNodes;
private final RoutingTable routingTable;
private final Settings settings;
private final IndexMetadata indexMetadata;
private final Logger logger;

public RemoteMigrationIndexMetadataUpdater(Logger logger) {
public RemoteMigrationIndexMetadataUpdater(
DiscoveryNodes discoveryNodes,
RoutingTable routingTable,
IndexMetadata indexMetadata,
Settings settings,
Logger logger

) {
this.discoveryNodes = discoveryNodes;
this.routingTable = routingTable;
this.settings = settings;
this.indexMetadata = indexMetadata;
this.logger = logger;
}

Expand All @@ -54,11 +69,8 @@ public RemoteMigrationIndexMetadataUpdater(Logger logger) {
* Also appends the requisite Remote Store Path based custom metadata to the existing index metadata
*/
public void maybeAddRemoteIndexSettings(
IndexMetadata indexMetadata,
IndexMetadata.Builder indexMetadataBuilder,
RoutingTable routingTable,
String index,
DiscoveryNodes discoveryNodes,
String segmentRepoName,
String tlogRepoName
) {
Expand Down Expand Up @@ -89,7 +101,7 @@ public void maybeAddRemoteIndexSettings(
* @param currentIndexSettings current {@link IndexMetadata} from cluster state
* @return <code>true</code> or <code>false</code> depending on the met conditions
*/
public boolean needsRemoteIndexSettingsUpdate(
private boolean needsRemoteIndexSettingsUpdate(
IndexRoutingTable indexRoutingTable,
DiscoveryNodes discoveryNodes,
Settings currentIndexSettings
Expand All @@ -116,19 +128,10 @@ public boolean needsRemoteIndexSettingsUpdate(
* is not used anywhere in the docrep flow
* Checks are in place to make this execution no-op if the index metadata is already present.
*
* @param indexMetadata Current {@link IndexMetadata}
* @param indexMetadataBuilder Mutated {@link IndexMetadata.Builder} having the previous state updates
* @param index index name
* @param discoveryNodes Current {@link DiscoveryNodes} from the cluster state
* @param settings current cluster settings from {@link ClusterState}
*/
public void maybeUpdateRemoteStorePathStrategy(
IndexMetadata indexMetadata,
IndexMetadata.Builder indexMetadataBuilder,
String index,
DiscoveryNodes discoveryNodes,
Settings settings
) {
public void maybeUpdateRemoteStorePathStrategy(IndexMetadata.Builder indexMetadataBuilder, String index) {
if (indexHasRemotePathMetadata(indexMetadata) == false) {
logger.info("Adding remote store path strategy for index [{}] during migration", index);
indexMetadataBuilder.putCustom(REMOTE_STORE_CUSTOM_KEY, createRemoteStorePathTypeMetadata(settings, discoveryNodes));
Expand All @@ -144,7 +147,7 @@ public void maybeUpdateRemoteStorePathStrategy(
* @param discoveryNodes Current {@link DiscoveryNodes} from the cluster state
* @return {@link Map} to be added as custom data in index metadata
*/
public Map<String, String> createRemoteStorePathTypeMetadata(Settings settings, DiscoveryNodes discoveryNodes) {
private Map<String, String> createRemoteStorePathTypeMetadata(Settings settings, DiscoveryNodes discoveryNodes) {
Version minNodeVersion = discoveryNodes.getMinNodeVersion();
PathType pathType = Version.CURRENT.compareTo(minNodeVersion) <= 0
? CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.get(settings)
Expand Down
125 changes: 57 additions & 68 deletions ...r/src/test/java/org/opensearch/index/remote/RemoteMigrationIndexMetadataUpdaterTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@
import org.opensearch.test.OpenSearchTestCase;

import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.UUID;

import static org.opensearch.cluster.metadata.IndexMetadata.REMOTE_STORE_CUSTOM_KEY;
import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING;
import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING;
import static org.mockito.Mockito.mock;

public class RemoteMigrationIndexMetadataUpdaterTests extends OpenSearchTestCase {
private final String tlogRepoName = "test-tlog-repo";
Expand All @@ -44,154 +44,152 @@ public class RemoteMigrationIndexMetadataUpdaterTests extends OpenSearchTestCase

public void testMaybeAddRemoteIndexSettingsAllPrimariesAndReplicasOnRemote() throws IOException {
Metadata metadata = createIndexMetadataWithDocrepSettings(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(metadata.index(indexName));
IndexMetadata existingIndexMetadata = metadata.index(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(existingIndexMetadata);
long currentSettingsVersion = indexMetadataBuilder.settingsVersion();
DiscoveryNode primaryNode = IndexShardTestUtils.getFakeRemoteEnabledNode("1");
DiscoveryNode replicaNode = IndexShardTestUtils.getFakeRemoteEnabledNode("2");
DiscoveryNodes allNodes = DiscoveryNodes.builder().add(primaryNode).add(replicaNode).build();
RoutingTable routingTable = createRoutingTableAllShardsStarted(indexName, 1, 1, primaryNode, replicaNode);
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(
metadata.index(indexName),
indexMetadataBuilder,
routingTable,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
allNodes,
segmentRepoName,
tlogRepoName
routingTable,
existingIndexMetadata,
existingIndexMetadata.getSettings(),
logger
);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
assertTrue(currentSettingsVersion < indexMetadataBuilder.settingsVersion());
assertRemoteSettingsApplied(indexMetadataBuilder.build());
}

public void testMaybeAddRemoteIndexSettingsDoesNotRunWhenSettingsAlreadyPresent() throws IOException {
Metadata metadata = createIndexMetadataWithRemoteStoreSettings(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(metadata.index(indexName));
IndexMetadata existingIndexMetadata = metadata.index(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(existingIndexMetadata);
long currentSettingsVersion = indexMetadataBuilder.settingsVersion();
DiscoveryNode primaryNode = IndexShardTestUtils.getFakeRemoteEnabledNode("1");
DiscoveryNode replicaNode = IndexShardTestUtils.getFakeRemoteEnabledNode("2");
DiscoveryNodes allNodes = DiscoveryNodes.builder().add(primaryNode).add(replicaNode).build();
RoutingTable routingTable = createRoutingTableAllShardsStarted(indexName, 1, 1, primaryNode, replicaNode);
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(
metadata.index(indexName),
indexMetadataBuilder,
routingTable,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
allNodes,
segmentRepoName,
tlogRepoName
routingTable,
existingIndexMetadata,
existingIndexMetadata.getSettings(),
logger
);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
assertEquals(currentSettingsVersion, indexMetadataBuilder.settingsVersion());
}

public void testMaybeAddRemoteIndexSettingsDoesNotUpdateSettingsWhenAllShardsInDocrep() throws IOException {
Metadata metadata = createIndexMetadataWithDocrepSettings(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(metadata.index(indexName));
IndexMetadata existingIndexMetadata = metadata.index(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(existingIndexMetadata);
long currentSettingsVersion = indexMetadataBuilder.settingsVersion();
DiscoveryNode primaryNode = IndexShardTestUtils.getFakeDiscoNode("1");
DiscoveryNode replicaNode = IndexShardTestUtils.getFakeDiscoNode("2");
DiscoveryNodes allNodes = DiscoveryNodes.builder().add(primaryNode).add(replicaNode).build();
RoutingTable routingTable = createRoutingTableAllShardsStarted(indexName, 1, 1, primaryNode, replicaNode);
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(
metadata.index(indexName),
indexMetadataBuilder,
routingTable,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
allNodes,
segmentRepoName,
tlogRepoName
routingTable,
existingIndexMetadata,
existingIndexMetadata.getSettings(),
logger
);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
assertEquals(currentSettingsVersion, indexMetadataBuilder.settingsVersion());
assertDocrepSettingsApplied(indexMetadataBuilder.build());
}

public void testMaybeAddRemoteIndexSettingsUpdatesIndexSettingsWithUnassignedReplicas() throws IOException {
Metadata metadata = createIndexMetadataWithDocrepSettings(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(metadata.index(indexName));
IndexMetadata existingIndexMetadata = metadata.index(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(existingIndexMetadata);
long currentSettingsVersion = indexMetadataBuilder.settingsVersion();
DiscoveryNode primaryNode = IndexShardTestUtils.getFakeRemoteEnabledNode("1");
DiscoveryNode replicaNode = IndexShardTestUtils.getFakeDiscoNode("2");
DiscoveryNodes allNodes = DiscoveryNodes.builder().add(primaryNode).add(replicaNode).build();
RoutingTable routingTable = createRoutingTableReplicasUnassigned(indexName, 1, 1, primaryNode);
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(
metadata.index(indexName),
indexMetadataBuilder,
routingTable,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
allNodes,
segmentRepoName,
tlogRepoName
routingTable,
existingIndexMetadata,
existingIndexMetadata.getSettings(),
logger
);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
assertTrue(currentSettingsVersion < indexMetadataBuilder.settingsVersion());
assertRemoteSettingsApplied(indexMetadataBuilder.build());
}

public void testMaybeAddRemoteIndexSettingsDoesNotUpdateIndexSettingsWithRelocatingReplicas() throws IOException {
Metadata metadata = createIndexMetadataWithDocrepSettings(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(metadata.index(indexName));
IndexMetadata existingIndexMetadata = metadata.index(indexName);
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(existingIndexMetadata);
long currentSettingsVersion = indexMetadataBuilder.settingsVersion();
DiscoveryNode primaryNode = IndexShardTestUtils.getFakeRemoteEnabledNode("1");
DiscoveryNode replicaNode = IndexShardTestUtils.getFakeDiscoNode("2");
DiscoveryNode replicaRelocatingNode = IndexShardTestUtils.getFakeDiscoNode("3");
DiscoveryNodes allNodes = DiscoveryNodes.builder().add(primaryNode).add(replicaNode).build();
RoutingTable routingTable = createRoutingTableReplicasRelocating(indexName, 1, 1, primaryNode, replicaNode, replicaRelocatingNode);
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(
metadata.index(indexName),
indexMetadataBuilder,
routingTable,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
allNodes,
segmentRepoName,
tlogRepoName
routingTable,
existingIndexMetadata,
existingIndexMetadata.getSettings(),
logger
);
migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
assertEquals(currentSettingsVersion, indexMetadataBuilder.settingsVersion());
assertDocrepSettingsApplied(indexMetadataBuilder.build());
}

public void testMaybeUpdateRemoteStorePathStrategyExecutes() {
Metadata currentMetadata = createIndexMetadataWithDocrepSettings(indexName);
IndexMetadata.Builder builder = IndexMetadata.builder(currentMetadata.index(indexName));
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
IndexMetadata existingIndexMetadata = currentMetadata.index(indexName);
IndexMetadata.Builder builder = IndexMetadata.builder(existingIndexMetadata);
DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(IndexShardTestUtils.getFakeRemoteEnabledNode("1")).build();
migrationIndexMetadataUpdater.maybeUpdateRemoteStorePathStrategy(
currentMetadata.index(indexName),
builder,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
discoveryNodes,
mock(RoutingTable.class),
existingIndexMetadata,
Settings.builder()
.put(
CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING.getKey(),
RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1.name()
)
.put(CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.getKey(), RemoteStoreEnums.PathType.HASHED_PREFIX.name())
.build()
.build(),
logger
);
migrationIndexMetadataUpdater.maybeUpdateRemoteStorePathStrategy(builder, indexName);
assertCustomPathMetadataIsPresent(builder.build());
}

public void testMaybeUpdateRemoteStorePathStrategyDoesNotExecute() {
Metadata currentMetadata = createIndexMetadataWithRemoteStoreSettings(indexName);
IndexMetadata existingIndexMetadata = currentMetadata.index(indexName);
IndexMetadata.Builder builder = IndexMetadata.builder(currentMetadata.index(indexName));
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(logger);
DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(IndexShardTestUtils.getFakeRemoteEnabledNode("1")).build();
migrationIndexMetadataUpdater.maybeUpdateRemoteStorePathStrategy(
currentMetadata.index(indexName),
builder,
indexName,
RemoteMigrationIndexMetadataUpdater migrationIndexMetadataUpdater = new RemoteMigrationIndexMetadataUpdater(
discoveryNodes,
mock(RoutingTable.class),
existingIndexMetadata,
Settings.builder()
.put(
CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING.getKey(),
RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1.name()
)
.put(CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.getKey(), RemoteStoreEnums.PathType.HASHED_PREFIX.name())
.build()
.build(),
logger
);

migrationIndexMetadataUpdater.maybeUpdateRemoteStorePathStrategy(builder, indexName);

assertCustomPathMetadataIsPresent(builder.build());
}

Expand Down Expand Up @@ -321,15 +319,6 @@ public static Metadata createIndexMetadataWithDocrepSettings(String indexName) {
return Metadata.builder().put(indexMetadata).build();
}

private static IndexMetadata createIndexMetadataWithCustomRemotePath(String indexName) {
IndexMetadata.Builder indexMetadata = IndexMetadata.builder(indexName);
Map<String, String> customRemotePathData = new HashMap<>();
customRemotePathData.put(RemoteStoreEnums.PathType.NAME, RemoteStoreEnums.PathType.FIXED.name());
customRemotePathData.put(RemoteStoreEnums.PathHashAlgorithm.NAME, RemoteStoreEnums.PathHashAlgorithm.FNV_1A_BASE64.name());
indexMetadata.putCustom(REMOTE_STORE_CUSTOM_KEY, customRemotePathData);
return indexMetadata.build();
}

private void assertRemoteSettingsApplied(IndexMetadata indexMetadata) {
assertTrue(IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING.get(indexMetadata.getSettings()));
assertTrue(IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.exists(indexMetadata.getSettings()));
Expand Down

0 comments on commit 6f9f0c3

Please sign in to comment.