Skip to content

Commit

Permalink
Updating the separator for RemoteStoreLockManager since underscore is…
Browse files Browse the repository at this point in the history
… allowed in base64UUID url charset (#10379)

* Refactor Remote Store Metadata Lock Manager Utils

Signed-off-by: Harish Bhakuni <[email protected]>

* Address PR Comments

Signed-off-by: Harish Bhakuni <[email protected]>

* Address PR Comments

Signed-off-by: Harish Bhakuni <[email protected]>

* Update Changelog entry

Signed-off-by: Harish Bhakuni <[email protected]>

* Update Changelog entry

Signed-off-by: Harish Bhakuni <[email protected]>

* Unmute testDeleteShallowCopySnapshot test

Signed-off-by: Harish Bhakuni <[email protected]>

---------

Signed-off-by: Harish Bhakuni <[email protected]>
Co-authored-by: Harish Bhakuni <[email protected]>
  • Loading branch information
harishbhakuni and Harish Bhakuni authored Oct 5, 2023
1 parent a3f8432 commit 66aef13
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 34 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
- Updating the separator for RemoteStoreLockManager since underscore is allowed in base64UUID url charset ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379))

### Deprecated

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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() + ".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);
Expand All @@ -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() + ".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);
Expand Down Expand Up @@ -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() + ".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);
Expand Down Expand Up @@ -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() + ".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(
Expand All @@ -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<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");
snapshotInfo = createSnapshot(
Expand All @@ -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);
Expand All @@ -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");
Expand Down

0 comments on commit 66aef13

Please sign in to comment.