Updating the separator for RemoteStoreLockManager since underscore is…

… allowed in base64UUID url charset (#10379) (#10428)

* Refactor Remote Store Metadata Lock Manager Utils



* Address PR Comments



* Address PR Comments



* Update Changelog entry



* Update Changelog entry



* Unmute testDeleteShallowCopySnapshot test



---------



(cherry picked from commit 66aef13)

Signed-off-by: Harish Bhakuni <hbhakuni@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Harish Bhakuni <hbhakuni@amazon.com>

CHANGELOG.md

@@ -51,6 +51,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Enable remote segment upload backpressure by default ([#10356](https://github.com/opensearch-project/OpenSearch/pull/10356))
 - [Remote Store] Add support to reload repository metadata inplace ([#9569](https://github.com/opensearch-project/OpenSearch/pull/9569))
 - [Metrics Framework] Add Metrics framework. ([#10241](https://github.com/opensearch-project/OpenSearch/pull/10241))
+- Updating the separator for RemoteStoreLockManager since underscore is allowed in base64UUID url charset ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379))
 
 ### Deprecated
 

server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java

@@ -21,6 +21,7 @@
 public class FileLockInfo implements LockInfo {
     private String fileToLock;
     private String acquirerId;
+    private static final int INVALID_INDEX = -1;
 
     public String getAcquirerId() {
         return acquirerId;
@@ -88,21 +89,34 @@ static String generateLockName(String fileToLock, String acquirerId) {
         }
 
         public static String getFileToLockNameFromLock(String lockName) {
-            String[] lockNameTokens = lockName.split(RemoteStoreLockManagerUtils.SEPARATOR);
-
-            if (lockNameTokens.length != 2) {
-                throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid.");
+            // use proper separator for the lock file depending on the version it is created
+            String lockSeparator = lockName.endsWith(RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION)
+                ? RemoteStoreLockManagerUtils.PRE_OS210_LOCK_SEPARATOR
+                : RemoteStoreLockManagerUtils.SEPARATOR;
+            final int indexOfSeparator = lockName.lastIndexOf(lockSeparator);
+            if (indexOfSeparator == INVALID_INDEX) {
+                throw new IllegalArgumentException("Provided lock name: " + lockName + " is invalid with separator: " + lockSeparator);
             }
-            return lockNameTokens[0];
+            return lockName.substring(0, indexOfSeparator);
         }
 
         public static String getAcquirerIdFromLock(String lockName) {
-            String[] lockNameTokens = lockName.split(RemoteStoreLockManagerUtils.SEPARATOR);
+            String lockExtension = RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION;
+            String lockSeparator = RemoteStoreLockManagerUtils.SEPARATOR;
 
-            if (lockNameTokens.length != 2) {
-                throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid.");
+            // check if lock file is created on version <=2.10
+            if (lockName.endsWith(RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION)) {
+                lockSeparator = RemoteStoreLockManagerUtils.PRE_OS210_LOCK_SEPARATOR;
+                lockExtension = RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION;
+            }
+            final int indexOfSeparator = lockName.lastIndexOf(lockSeparator);
+            final int indexOfExt = lockName.lastIndexOf(lockExtension);
+            if (indexOfSeparator == INVALID_INDEX || indexOfExt == INVALID_INDEX) {
+                throw new IllegalArgumentException(
+                    "Provided lock name: " + lockName + " is invalid with separator: " + lockSeparator + " and extension: " + lockExtension
+                );
             }
-            return lockNameTokens[1].replace(RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION, "");
+            return lockName.substring(indexOfSeparator + lockSeparator.length(), indexOfExt);
         }
     }
 

server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java

@@ -15,8 +15,11 @@
  */
 public class RemoteStoreLockManagerUtils {
     static final String FILE_TO_LOCK_NAME = "file_to_lock";
-    static final String SEPARATOR = "___";
-    static final String LOCK_FILE_EXTENSION = ".lock";
+    static final String PRE_OS210_LOCK_SEPARATOR = "___";
+    static final String SEPARATOR = "...";
+    // for versions <= 2.10, we have lock files with this extension.
+    static final String PRE_OS210_LOCK_FILE_EXTENSION = ".lock";
+    static final String LOCK_FILE_EXTENSION = ".v2_lock";
     static final String ACQUIRER_ID = "acquirer_id";
     public static final String NO_TTL = "-1";
     static final String LOCK_EXPIRY_TIME = "lock_expiry_time";

server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java

@@ -15,10 +15,24 @@
 public class FileLockInfoTests extends OpenSearchTestCase {
     String testMetadata = "testMetadata";
     String testAcquirerId = "testAcquirerId";
+    String testAcquirerId2 = "ZxZ4Wh89SXyEPmSYAHrIrQ";
+    String testAcquirerId3 = "ZxZ4Wh89SXyEPmSYAHrItS";
+    String testMetadata1 = "metadata__9223372036854775806__9223372036854775803__9223372036854775790"
+        + "__9223372036854775800___Hf3Dbw2QQagfGLlVBOUrg__9223370340398865071__1";
+
+    String oldLock = testMetadata1 + RemoteStoreLockManagerUtils.PRE_OS210_LOCK_SEPARATOR + testAcquirerId2
+        + RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION;
+    String newLock = testMetadata1 + RemoteStoreLockManagerUtils.SEPARATOR + testAcquirerId3
+        + RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION;
 
     public void testGenerateLockName() {
         FileLockInfo fileLockInfo = FileLockInfo.getLockInfoBuilder().withFileToLock(testMetadata).withAcquirerId(testAcquirerId).build();
         assertEquals(fileLockInfo.generateLockName(), FileLockInfo.LockFileUtils.generateLockName(testMetadata, testAcquirerId));
+
+        // validate that lock generated will be the new version lock
+        fileLockInfo = FileLockInfo.getLockInfoBuilder().withFileToLock(testMetadata1).withAcquirerId(testAcquirerId3).build();
+        assertEquals(fileLockInfo.generateLockName(), newLock);
+
     }
 
     public void testGenerateLockNameFailureCase1() {
@@ -41,13 +55,33 @@ public void testGetLockPrefixFailureCase() {
         assertThrows(IllegalArgumentException.class, fileLockInfo::getLockPrefix);
     }
 
+    public void testGetFileToLockNameFromLock() {
+        assertEquals(testMetadata1, FileLockInfo.LockFileUtils.getFileToLockNameFromLock(oldLock));
+        assertEquals(testMetadata1, FileLockInfo.LockFileUtils.getFileToLockNameFromLock(newLock));
+    }
+
+    public void testGetAcquirerIdFromLock() {
+        assertEquals(testAcquirerId2, FileLockInfo.LockFileUtils.getAcquirerIdFromLock(oldLock));
+        assertEquals(testAcquirerId3, FileLockInfo.LockFileUtils.getAcquirerIdFromLock(newLock));
+    }
+
     public void testGetLocksForAcquirer() throws NoSuchFileException {
+
         String[] locks = new String[] {
             FileLockInfo.LockFileUtils.generateLockName(testMetadata, testAcquirerId),
-            FileLockInfo.LockFileUtils.generateLockName(testMetadata, "acquirerId2") };
+            FileLockInfo.LockFileUtils.generateLockName(testMetadata, "acquirerId2"),
+            oldLock,
+            newLock };
         FileLockInfo fileLockInfo = FileLockInfo.getLockInfoBuilder().withAcquirerId(testAcquirerId).build();
-
         assertEquals(fileLockInfo.getLockForAcquirer(locks), FileLockInfo.LockFileUtils.generateLockName(testMetadata, testAcquirerId));
+
+        // validate old lock
+        fileLockInfo = FileLockInfo.getLockInfoBuilder().withAcquirerId(testAcquirerId2).build();
+        assertEquals(fileLockInfo.getLockForAcquirer(locks), oldLock);
+
+        // validate new lock
+        fileLockInfo = FileLockInfo.getLockInfoBuilder().withAcquirerId(testAcquirerId3).build();
+        assertEquals(fileLockInfo.getLockForAcquirer(locks), newLock);
     }
 
 }

server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java

@@ -59,7 +59,9 @@ protected String[] getLockFilesInRemoteStore(String remoteStoreIndex, String rem
         BlobPath shardLevelBlobPath = remoteStorerepository.basePath().add(indexUUID).add("0").add("segments").add("lock_files");
         BlobContainer blobContainer = remoteStorerepository.blobStore().blobContainer(shardLevelBlobPath);
         try (RemoteBufferedOutputDirectory lockDirectory = new RemoteBufferedOutputDirectory(blobContainer)) {
-            return Arrays.stream(lockDirectory.listAll()).filter(lock -> lock.endsWith(".lock")).toArray(String[]::new);
+            return Arrays.stream(lockDirectory.listAll())
+                .filter(lock -> lock.endsWith(".lock") || lock.endsWith(".v2_lock"))
+                .toArray(String[]::new);
         }
     }
 

server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java

@@ -144,7 +144,7 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException {
         final SnapshotId snapshotId1 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 0) : "there should be no lock files present in directory, but found " + Arrays.toString(lockFiles);
+        assertEquals("there should be no lock files present in directory, but found " + Arrays.toString(lockFiles), 0, lockFiles.length);
         logger.info("--> create remote index shallow snapshot");
         Settings snapshotRepoSettingsForShallowCopy = Settings.builder()
             .put(snapshotRepoSettings)
@@ -161,8 +161,8 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException {
         final SnapshotId snapshotId2 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "there should be only one lock file, but found " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId2.getUUID() + ".lock");
+        assertEquals("there should be only one lock file, but found " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId2.getUUID() + ".v2_lock"));
 
         logger.info("--> create another normal snapshot");
         updateRepository(client, snapshotRepositoryName, snapshotRepoSettings);
@@ -175,8 +175,8 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException {
         final SnapshotId snapshotId3 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "there should be only one lock file, but found " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId2.getUUID() + ".lock");
+        assertEquals("there should be only one lock file, but found " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId2.getUUID() + ".v2_lock"));
 
         logger.info("--> make sure the node's repository can resolve the snapshots");
         final List<SnapshotId> originalSnapshots = Arrays.asList(snapshotId1, snapshotId2, snapshotId3);
@@ -236,8 +236,8 @@ public void testGetRemoteStoreShallowCopyShardMetadata() throws IOException {
         final SnapshotId snapshotId = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "there should be only one lock file, but found " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId.getUUID() + ".lock");
+        assertEquals("there should be only one lock file, but found " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId.getUUID() + ".v2_lock"));
 
         final RepositoriesService repositoriesService = getInstanceFromNode(RepositoriesService.class);
         final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(snapshotRepositoryName);
@@ -312,8 +312,8 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId1 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "lock files are " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId1.getUUID() + ".lock");
+        assertEquals("lock files are " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId1.getUUID() + ".v2_lock"));
 
         logger.info("--> create second remote index shallow snapshot");
         createSnapshotResponse = client.admin()
@@ -325,10 +325,10 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId2 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 2) : "lock files are " + Arrays.toString(lockFiles);
+        assertEquals("lock files are " + Arrays.toString(lockFiles), 2, lockFiles.length);
         List<SnapshotId> shallowCopySnapshotIDs = Arrays.asList(snapshotId1, snapshotId2);
         for (SnapshotId snapshotId : shallowCopySnapshotIDs) {
-            assert lockFiles[0].contains(snapshotId.getUUID()) || lockFiles[1].contains(snapshotId.getUUID());
+            assertTrue(lockFiles[0].contains(snapshotId.getUUID()) || lockFiles[1].contains(snapshotId.getUUID()));
         }
         logger.info("--> create third remote index shallow snapshot");
         createSnapshotResponse = client.admin()
@@ -340,12 +340,14 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId3 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 3);
+        assertEquals(3, lockFiles.length);
         shallowCopySnapshotIDs = Arrays.asList(snapshotId1, snapshotId2, snapshotId3);
         for (SnapshotId snapshotId : shallowCopySnapshotIDs) {
-            assert lockFiles[0].contains(snapshotId.getUUID())
-                || lockFiles[1].contains(snapshotId.getUUID())
-                || lockFiles[2].contains(snapshotId.getUUID());
+            assertTrue(
+                lockFiles[0].contains(snapshotId.getUUID())
+                    || lockFiles[1].contains(snapshotId.getUUID())
+                    || lockFiles[2].contains(snapshotId.getUUID())
+            );
         }
         logger.info("--> create normal snapshot");
         createRepository(client, snapshotRepositoryName, snapshotRepoSettings);
@@ -358,12 +360,14 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId4 = createSnapshotResponse.getSnapshotInfo().snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 3) : "lock files are " + Arrays.toString(lockFiles);
+        assertEquals("lock files are " + Arrays.toString(lockFiles), 3, lockFiles.length);
         shallowCopySnapshotIDs = Arrays.asList(snapshotId1, snapshotId2, snapshotId3);
         for (SnapshotId snapshotId : shallowCopySnapshotIDs) {
-            assert lockFiles[0].contains(snapshotId.getUUID())
-                || lockFiles[1].contains(snapshotId.getUUID())
-                || lockFiles[2].contains(snapshotId.getUUID());
+            assertTrue(
+                lockFiles[0].contains(snapshotId.getUUID())
+                    || lockFiles[1].contains(snapshotId.getUUID())
+                    || lockFiles[2].contains(snapshotId.getUUID())
+            );
         }
 
         logger.info("--> make sure the node's repository can resolve the snapshots");