Incoporate PR review feedback

Signed-off-by: Ashish Singh <[email protected]>

server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java

@@ -527,17 +527,17 @@ private List<UploadedIndexMetadata> writeIndexMetadataParallel(
      * Invokes the index metadata upload listener but does not wait for the execution to complete.
      */
     private void invokeIndexMetadataUploadListeners(
-        List<IndexMetadata> udpatedIndexMetadataList,
+        List<IndexMetadata> updatedIndexMetadataList,
         Map<String, IndexMetadata> prevIndexMetadataByName,
         CountDownLatch latch,
         List<Exception> exceptionList
     ) {
         for (IndexMetadataUploadListener listener : indexMetadataUploadListeners) {
             String listenerName = listener.getClass().getSimpleName();
             listener.onUpload(
-                udpatedIndexMetadataList,
+                updatedIndexMetadataList,
                 prevIndexMetadataByName,
-                getIndexMetadataUploadActionListener(udpatedIndexMetadataList, prevIndexMetadataByName, latch, exceptionList, listenerName)
+                getIndexMetadataUploadActionListener(updatedIndexMetadataList, prevIndexMetadataByName, latch, exceptionList, listenerName)
             );
         }
 

server/src/main/java/org/opensearch/index/IndexSettings.java

@@ -987,7 +987,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
          */
         widenIndexSortType = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(settings).before(V_2_7_0);
         assignedOnRemoteNode = RemoteStoreNodeAttribute.isRemoteDataAttributePresent(this.getNodeSettings());
-        remoteStorePathStrategy = RemoteStoreUtils.determineRemoteStorePathStrategy(indexMetadata, true);
+        remoteStorePathStrategy = RemoteStoreUtils.determineRemoteStorePathStrategy(indexMetadata);
 
         setEnableFuzzySetForDocId(scopedSettings.get(INDEX_DOC_ID_FUZZY_SET_ENABLED_SETTING));
         setDocIdFuzzySetFalsePositiveProbability(scopedSettings.get(INDEX_DOC_ID_FUZZY_SET_FALSE_POSITIVE_PROBABILITY_SETTING));

server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java

@@ -279,10 +279,8 @@ private LatchedActionListener<Void> getUploadPathLatchedActionListener(
      * uploads. It checks if the remote store path type is {@code HASHED_PREFIX} and returns true if so.
      */
     private boolean requiresPathUpload(IndexMetadata indexMetadata, IndexMetadata prevIndexMetadata) {
-        PathType pathType = determineRemoteStorePathStrategy(indexMetadata, false).getType();
-        PathType prevPathType = Objects.nonNull(prevIndexMetadata)
-            ? determineRemoteStorePathStrategy(prevIndexMetadata, false).getType()
-            : null;
+        PathType pathType = determineRemoteStorePathStrategy(indexMetadata).getType();
+        PathType prevPathType = Objects.nonNull(prevIndexMetadata) ? determineRemoteStorePathStrategy(prevIndexMetadata).getType() : null;
         // If previous metadata is null or previous path type is not hashed_prefix, and along with new path type being
         // hashed_prefix, then this can mean any of the following -
         // 1. This is creation of remote index with hashed_prefix
@@ -297,7 +295,10 @@ private void setIndexMetadataUploadTimeout(TimeValue newIndexMetadataUploadTimeo
 
     /**
      * Creates a file name by combining index uuid, index metadata version and file version. # has been chosen as the
-     * delimiter since it does not collide with any possible letters in file name.
+     * delimiter since it does not collide with any possible letters in file name. The random base64 uuid is added to
+     * ensure that the file does not get overwritten. We do check if translog and segment repo are same by name, but
+     * it is possible that a user configures same repo by different name for translog and segment in which case, this
+     * will lead to file not being overwritten.
      */
     private String generateFileName(String indexUUID, long indexMetadataVersion, String fileVersion) {
         return String.join(DELIMITER, indexUUID, Long.toString(indexMetadataVersion), fileVersion, UUIDs.randomBase64UUID());

server/src/main/java/org/opensearch/index/remote/RemoteStoreUtils.java

@@ -152,11 +152,9 @@ static String longToCompositeBase64AndBinaryEncoding(long value, int len) {
     /**
      * Determines the remote store path strategy by reading the custom data map in IndexMetadata class.
      */
-    public static RemoteStorePathStrategy determineRemoteStorePathStrategy(IndexMetadata indexMetadata, boolean assertRemoteCustomData) {
+    public static RemoteStorePathStrategy determineRemoteStorePathStrategy(IndexMetadata indexMetadata) {
         Map<String, String> remoteCustomData = indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY);
-        if (assertRemoteCustomData) {
-            assert remoteCustomData == null || remoteCustomData.containsKey(RemoteStoreEnums.PathType.NAME);
-        }
+        assert remoteCustomData == null || remoteCustomData.containsKey(RemoteStoreEnums.PathType.NAME);
         if (remoteCustomData != null && remoteCustomData.containsKey(RemoteStoreEnums.PathType.NAME)) {
             RemoteStoreEnums.PathType pathType = RemoteStoreEnums.PathType.parseString(
                 remoteCustomData.get(RemoteStoreEnums.PathType.NAME)

server/src/test/java/org/opensearch/index/remote/RemoteIndexPathUploaderTests.java

@@ -180,15 +180,23 @@ public void testInterceptWithEmptyEligibleIndexMetadataList() {
         assertEquals(0, failureCount.get());
 
         // Case 2 - Empty remoteCustomData
-        indexMetadataList = List.of(createIndexMetadata(new HashMap<>()));
-        remoteIndexPathUploader.doOnUpload(indexMetadataList, Collections.emptyMap(), actionListener);
-        assertEquals(2, successCount.get());
+        assertThrows(
+            AssertionError.class,
+            () -> remoteIndexPathUploader.doOnUpload(List.of(createIndexMetadata(new HashMap<>())), Collections.emptyMap(), actionListener)
+        );
+        assertEquals(1, successCount.get());
         assertEquals(0, failureCount.get());
 
         // Case 3 - RemoteStoreEnums.PathType.NAME not in remoteCustomData map
-        indexMetadataList = List.of(createIndexMetadata(Map.of("test", "test")));
-        remoteIndexPathUploader.doOnUpload(indexMetadataList, Collections.emptyMap(), actionListener);
-        assertEquals(3, successCount.get());
+        assertThrows(
+            AssertionError.class,
+            () -> remoteIndexPathUploader.doOnUpload(
+                List.of(createIndexMetadata(Map.of("test", "test"))),
+                Collections.emptyMap(),
+                actionListener
+            )
+        );
+        assertEquals(1, successCount.get());
         assertEquals(0, failureCount.get());
 
         // Case 4 - RemoteStoreEnums.PathType.NAME is not HASHED_PREFIX
@@ -199,7 +207,7 @@ public void testInterceptWithEmptyEligibleIndexMetadataList() {
         remoteCustomData.put(PathHashAlgorithm.NAME, pathHashAlgorithm);
         indexMetadataList = List.of(createIndexMetadata(remoteCustomData));
         remoteIndexPathUploader.doOnUpload(indexMetadataList, Collections.emptyMap(), actionListener);
-        assertEquals(4, successCount.get());
+        assertEquals(2, successCount.get());
         assertEquals(0, failureCount.get());
     }