From 8a1c4e7071cd10ea124b514429411dba8b1a96c5 Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Wed, 4 Oct 2023 21:30:40 -0700 Subject: [PATCH 1/6] Refactor Remote Store Metadata Lock Manager Utils Signed-off-by: Harish Bhakuni --- .../index/store/lockmanager/FileLockInfo.java | 35 +++++++++++++++++-- .../RemoteStoreLockManagerUtils.java | 7 ++-- .../store/lockmanager/FileLockInfoTests.java | 28 +++++++++++++-- 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java b/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java index 24f42743e1a04..2aea9900fe13a 100644 --- a/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java +++ b/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java @@ -11,6 +11,7 @@ import java.nio.file.NoSuchFileException; import java.util.Arrays; import java.util.List; +import java.util.regex.Pattern; import java.util.stream.Collectors; /** @@ -21,6 +22,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,7 +90,11 @@ static String generateLockName(String fileToLock, String acquirerId) { } public static String getFileToLockNameFromLock(String lockName) { - String[] lockNameTokens = lockName.split(RemoteStoreLockManagerUtils.SEPARATOR); + if (lockName.endsWith(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION)) { + // this is the old lock file created for versions <= 2.10 + return getFileToLockNameFromV1Lock(lockName); + } + String[] lockNameTokens = lockName.split(Pattern.quote(RemoteStoreLockManagerUtils.SEPARATOR)); if (lockNameTokens.length != 2) { throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); @@ -97,13 +103,38 @@ public static String getFileToLockNameFromLock(String lockName) { } public static String getAcquirerIdFromLock(String lockName) { - String[] lockNameTokens = lockName.split(RemoteStoreLockManagerUtils.SEPARATOR); + if (lockName.endsWith(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION)) { + // this is the old lock file created for versions <= 2.10 + return getAcquirerIdFromV1Lock(lockName); + } + String[] lockNameTokens = lockName.split(Pattern.quote(RemoteStoreLockManagerUtils.SEPARATOR)); if (lockNameTokens.length != 2) { throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); } return lockNameTokens[1].replace(RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION, ""); } + + private static String getFileToLockNameFromV1Lock(String lockName) { + if (lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR) == INVALID_INDEX + || lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION) == INVALID_INDEX) { + throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); + } + String acquirerId = getAcquirerIdFromV1Lock(lockName); + return lockName.substring(0, lockName.lastIndexOf(acquirerId)); + } + + private static String getAcquirerIdFromV1Lock(String lockName) { + if (lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR) == INVALID_INDEX + || lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION) == INVALID_INDEX) { + throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); + } + return lockName.substring( + lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR) + RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR + .length(), + lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION) + ); + } } /** diff --git a/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java b/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java index 452dfc329d88b..d0944ebe0cc98 100644 --- a/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java +++ b/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 V1_LOCK_SEPARATOR = "___"; + static final String SEPARATOR = "..."; + // for versions <= 2.10, we have lock files with this extension. + static final String V1_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"; diff --git a/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java b/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java index f3a2f1859923e..d22199a9e6443 100644 --- a/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java +++ b/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.V1_LOCK_SEPARATOR + testAcquirerId2 + + RemoteStoreLockManagerUtils.V1_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() { @@ -42,12 +56,22 @@ public void testGetLockPrefixFailureCase() { } 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); } } From 0bf63287b790730a25efaeeafef1bff303e7919e Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Thu, 5 Oct 2023 01:46:26 -0700 Subject: [PATCH 2/6] Address PR Comments Signed-off-by: Harish Bhakuni --- .../index/store/lockmanager/FileLockInfo.java | 59 +++++++------------ .../RemoteStoreLockManagerUtils.java | 4 +- .../store/lockmanager/FileLockInfoTests.java | 14 ++++- .../BlobStoreRepositoryHelperTests.java | 4 +- .../BlobStoreRepositoryRemoteIndexTests.java | 8 +-- 5 files changed, 42 insertions(+), 47 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java b/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java index 2aea9900fe13a..b6be60c489a6c 100644 --- a/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java +++ b/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java @@ -11,7 +11,6 @@ import java.nio.file.NoSuchFileException; import java.util.Arrays; import java.util.List; -import java.util.regex.Pattern; import java.util.stream.Collectors; /** @@ -90,50 +89,34 @@ static String generateLockName(String fileToLock, String acquirerId) { } public static String getFileToLockNameFromLock(String lockName) { - if (lockName.endsWith(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION)) { - // this is the old lock file created for versions <= 2.10 - return getFileToLockNameFromV1Lock(lockName); + // 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); } - String[] lockNameTokens = lockName.split(Pattern.quote(RemoteStoreLockManagerUtils.SEPARATOR)); - - if (lockNameTokens.length != 2) { - throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); - } - return lockNameTokens[0]; + return lockName.substring(0, indexOfSeparator); } public static String getAcquirerIdFromLock(String lockName) { - if (lockName.endsWith(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION)) { - // this is the old lock file created for versions <= 2.10 - return getAcquirerIdFromV1Lock(lockName); - } - String[] lockNameTokens = lockName.split(Pattern.quote(RemoteStoreLockManagerUtils.SEPARATOR)); - - if (lockNameTokens.length != 2) { - throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); - } - return lockNameTokens[1].replace(RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION, ""); - } + String lockExtension = RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION; + String lockSeparator = RemoteStoreLockManagerUtils.SEPARATOR; - private static String getFileToLockNameFromV1Lock(String lockName) { - if (lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR) == INVALID_INDEX - || lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION) == INVALID_INDEX) { - 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; } - String acquirerId = getAcquirerIdFromV1Lock(lockName); - return lockName.substring(0, lockName.lastIndexOf(acquirerId)); - } - - private static String getAcquirerIdFromV1Lock(String lockName) { - if (lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR) == INVALID_INDEX - || lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION) == INVALID_INDEX) { - throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid."); + 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 lockName.substring( - lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR) + RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR - .length(), - lockName.lastIndexOf(RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION) - ); + return lockName.substring(indexOfSeparator + lockSeparator.length(), indexOfExt); } } diff --git a/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java b/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java index d0944ebe0cc98..d5fb2722a64dc 100644 --- a/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java +++ b/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java @@ -15,10 +15,10 @@ */ public class RemoteStoreLockManagerUtils { static final String FILE_TO_LOCK_NAME = "file_to_lock"; - static final String V1_LOCK_SEPARATOR = "___"; + 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 V1_LOCK_FILE_EXTENSION = ".lock"; + 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"; diff --git a/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java b/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java index d22199a9e6443..80413d4cb6612 100644 --- a/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java +++ b/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java @@ -20,8 +20,8 @@ public class FileLockInfoTests extends OpenSearchTestCase { String testMetadata1 = "metadata__9223372036854775806__9223372036854775803__9223372036854775790" + "__9223372036854775800___Hf3Dbw2QQagfGLlVBOUrg__9223370340398865071__1"; - String oldLock = testMetadata1 + RemoteStoreLockManagerUtils.V1_LOCK_SEPARATOR + testAcquirerId2 - + RemoteStoreLockManagerUtils.V1_LOCK_FILE_EXTENSION; + 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; @@ -55,6 +55,16 @@ 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[] { diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java index 4e60cddd14d73..57c126b85ff70 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java @@ -66,7 +66,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); } } diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java index e3e1bf31e82dc..acbddde35e034 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java @@ -162,7 +162,7 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException { 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"); + assert lockFiles[0].endsWith(snapshotId2.getUUID() + ".v2_lock"); logger.info("--> create another normal snapshot"); updateRepository(client, snapshotRepositoryName, snapshotRepoSettings); @@ -175,7 +175,7 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException { 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"); + assert lockFiles[0].endsWith(snapshotId2.getUUID() + ".v2_lock"); logger.info("--> make sure the node's repository can resolve the snapshots"); final List originalSnapshots = Arrays.asList(snapshotId1, snapshotId2, snapshotId3); @@ -231,7 +231,7 @@ public void testGetRemoteStoreShallowCopyShardMetadata() throws IOException { 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"); + assert lockFiles[0].endsWith(snapshotId.getUUID() + ".v2_lock"); final RepositoriesService repositoriesService = getInstanceFromNode(RepositoriesService.class); final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(snapshotRepositoryName); @@ -306,7 +306,7 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException { String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName); assert (lockFiles.length == 1) : "lock files are " + Arrays.toString(lockFiles); - assert lockFiles[0].endsWith(snapshotId1.getUUID() + ".lock"); + assert lockFiles[0].endsWith(snapshotId1.getUUID() + ".v2_lock"); logger.info("--> create second remote index shallow snapshot"); snapshotInfo = createSnapshot( From 6f81d3c430b97d92dd5c4f394a7da58c211e4ab9 Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Thu, 5 Oct 2023 09:24:30 -0700 Subject: [PATCH 3/6] Address PR Comments Signed-off-by: Harish Bhakuni --- .../BlobStoreRepositoryRemoteIndexTests.java | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java index acbddde35e034..9cca495cced72 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java @@ -145,7 +145,7 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException { final SnapshotId snapshotId1 = snapshotInfo.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 = snapshotInfo.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() + ".v2_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); @@ -174,8 +174,8 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException { final SnapshotId snapshotId3 = snapshotInfo.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() + ".v2_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 originalSnapshots = Arrays.asList(snapshotId1, snapshotId2, snapshotId3); @@ -230,8 +230,8 @@ public void testGetRemoteStoreShallowCopyShardMetadata() throws IOException { final SnapshotId snapshotId = snapshotInfo.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() + ".v2_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); @@ -305,8 +305,8 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException { final SnapshotId snapshotId1 = snapshotInfo.snapshotId(); String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName); - assert (lockFiles.length == 1) : "lock files are " + Arrays.toString(lockFiles); - assert lockFiles[0].endsWith(snapshotId1.getUUID() + ".v2_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"); snapshotInfo = createSnapshot( @@ -317,10 +317,10 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException { final SnapshotId snapshotId2 = snapshotInfo.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 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"); snapshotInfo = createSnapshot( @@ -331,12 +331,14 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException { final SnapshotId snapshotId3 = snapshotInfo.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); @@ -348,12 +350,14 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException { final SnapshotId snapshotId4 = snapshotInfo.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"); From ef55857151a5e9ba95773c9db9eb2b145a8bc8ab Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Thu, 5 Oct 2023 11:01:54 -0700 Subject: [PATCH 4/6] Update Changelog entry Signed-off-by: Harish Bhakuni --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d82f15198878..8f8a5ed5059e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -132,6 +132,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)) +- Refactor Remote Store Metadata Lock Manager Utils ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379)) ### Deprecated From 847168862e4e63aca3f9c832c539adc48f92bdcf Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Thu, 5 Oct 2023 12:31:28 -0700 Subject: [PATCH 5/6] Update Changelog entry Signed-off-by: Harish Bhakuni --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f8a5ed5059e1..011156010e0b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -132,7 +132,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)) -- Refactor Remote Store Metadata Lock Manager Utils ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379)) +- Updating the separator for RemoteLockManager since underscore is allowed in base64UUID url charset ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379)) ### Deprecated From b8adac06c310c9239878744a3eacb66106754d72 Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Thu, 5 Oct 2023 12:39:44 -0700 Subject: [PATCH 6/6] Unmute testDeleteShallowCopySnapshot test Signed-off-by: Harish Bhakuni --- CHANGELOG.md | 2 +- .../java/org/opensearch/snapshots/DeleteSnapshotIT.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 011156010e0b7..57e7e9d924057 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -132,7 +132,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 RemoteLockManager since underscore is allowed in base64UUID url charset ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379)) +- Updating the separator for RemoteStoreLockManager since underscore is allowed in base64UUID url charset ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379)) ### Deprecated diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java index 31abc16bba50e..e79bf1c16b586 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java @@ -64,7 +64,6 @@ public void testDeleteSnapshot() throws Exception { assert (getRepositoryData(snapshotRepoName).getSnapshotIds().size() == 0); } - @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/9115") public void testDeleteShallowCopySnapshot() throws Exception { disableRepoConsistencyCheck("Remote store repository is being used in the test"); final Path remoteStoreRepoPath = randomRepoPath();