Skip to content

Commit

Permalink
PR comments changes
Browse files Browse the repository at this point in the history
  • Loading branch information
Jai Balani committed Dec 20, 2024
1 parent 2685135 commit 1b97cc7
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 44 deletions.
4 changes: 2 additions & 2 deletions ambry-store/src/main/java/com/github/ambry/store/DiskManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,11 @@ boolean addBlobStoreForFileCopy(ReplicaId replica) {
// create a bootstrap-in-progress file to distinguish it from regular stores (the file will be checked during
// BOOTSTRAP -> STANDBY transition)
createBootstrapFileIfAbsent(replica);
logger.info("New store is successfully added into DiskManager.");
logger.info("New store for partitionId {} is successfully added into DiskManager.", replica.getPartitionId());
succeed = true;
}
} catch (Exception e) {
logger.error("Failed to start new added store {} or add requirements to disk allocator", replica.getPartitionId(),
logger.error("Failed to start new added store for partitionId {} for FileCopy based replication", replica.getPartitionId(),
e);
} finally {
rwLock.writeLock().unlock();
Expand Down
16 changes: 11 additions & 5 deletions ambry-store/src/main/java/com/github/ambry/store/StorageManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,16 +543,17 @@ DiskManager addDisk(DiskId diskId) {
*/
@Override
public boolean addBlobStoreForFileCopy(ReplicaId replica) {
if (partitionToDiskManager.containsKey(replica.getPartitionId())) {
logger.info("{} already exists in storage manager, rejecting adding store request", replica.getPartitionId());
if (!partitionToDiskManager.containsKey(replica.getPartitionId())) {
logger.info("PartitionId {} doesn't exist in storage manager during state build, rejecting adding store request", replica.getPartitionId());
return false;
}
DiskManager diskManager = addDisk(replica.getDiskId());
// We don't require addDisk since DiskManager is already started during initialization of StorageManager as part
// of prefilecopy steps. We will fetch it from partitionToDiskManager map.
DiskManager diskManager = partitionToDiskManager.get(replica.getPartitionId());
if (diskManager == null || !diskManager.addBlobStoreForFileCopy(replica)) {
logger.error("Failed to add new store into DiskManager");
return false;
}
partitionToDiskManager.put(replica.getPartitionId(), diskManager);
partitionNameToReplicaId.put(replica.getPartitionId().toPathString(), replica);
logger.info("New store is successfully added into StorageManager");
return true;
Expand Down Expand Up @@ -600,9 +601,14 @@ public void buildStateForFileCopy(ReplicaId replica){
throw new StateTransitionException("Failed to add store for replica " + partitionId.getId() + " into storage manager",
ReplicaOperationFailure);
} else {
logger.info("Failed to add store for replica {} at location {}. Retrying bootstrapping replica at different location",
logger.info("Failed to add store for replica {} at location {}. Cleanup and raise StateTransitionException",
partitionId.getId(), replica.getReplicaPath());
// This will remove the reserved space from diskSpaceAllocator
tryRemoveFailedBootstrapBlobStore(replica);
// Throwing StateTransitionException here since we cannot retry adding BlobStore since Filecopy has copied data
// into the selected disk itself. Hence, putting the replica into ERROR state via StateTransitionException
throw new StateTransitionException("Failed to add store for replica " + partitionId.getId() + " into storage manager",
ReplicaOperationFailure);
}
}
Store store = getStore(replica.getPartitionId(), false);
Expand Down
74 changes: 37 additions & 37 deletions ambry-store/src/test/java/com/github/ambry/store/StorageManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,27 +294,34 @@ public MockId addRandomBlobToStore(Store store, long size, long expiresAtMs) thr
return id;
}

/**
* Test buildStateForFileCopy with newly created {@link ReplicaId}.
* @throws Exception
*/
@Test
public void buildStateForFileCopyTest() throws Exception {
private StorageManager initializeStorageManagerForStateBuildTests(int newMountPathIndex) throws Exception {
generateConfigs(true, false);
MockDataNodeId localNode = clusterMap.getDataNodes().get(0);
int newMountPathIndex = 3;
// add new MountPath to local node
File f = File.createTempFile("ambry", ".tmp");
File mountFile =
new File(f.getParent(), "mountpathfile" + MockClusterMap.PLAIN_TEXT_PORT_START_NUMBER + newMountPathIndex);
MockClusterMap.deleteFileOrDirectory(mountFile);
assertTrue("Couldn't create mount path directory", mountFile.mkdir());
localNode.addMountPaths(Collections.singletonList(mountFile.getAbsolutePath()));

StorageManager storageManager = createStorageManager(localNode, metricRegistry, null);
storageManager.start();
return storageManager;
}

/**
* Test buildStateForFileCopy with newly created {@link ReplicaId}.
* @throws Exception
*/
@Test
public void buildStateForFileCopyTest() throws Exception {
int newMountPathIndex = 3;
int newPartitionId = 803;
StorageManager storageManager = initializeStorageManagerForStateBuildTests(newMountPathIndex);
PartitionId newPartition =
new MockPartitionId(newPartitionId, MockClusterMap.DEFAULT_PARTITION_CLASS, clusterMap.getDataNodes(), newMountPathIndex);
StorageManager storageManager = createStorageManager(localNode, metricRegistry, null);
storageManager.start();
MockDataNodeId localNode = clusterMap.getDataNodes().get(0);
// test add store onto a new disk, which should succeed
assertTrue("Add new store should succeed", storageManager.addBlobStore(newPartition.getReplicaIds().get(0)));
assertNotNull("The store shouldn't be null because new store is successfully added",
Expand All @@ -323,10 +330,10 @@ public void buildStateForFileCopyTest() throws Exception {
Store store = dm.getStore(newPartition, false);
MockId id1 = addRandomBlobToStore(store, 100, Utils.Infinite_Time);
MockId id2 = addRandomBlobToStore(store, 200, Utils.Infinite_Time);
// Create storage manager again to create disk managers again
storageManager.shutdown();
storageManager = createStorageManager(localNode, metricRegistry, null);
storageManager.start();

// Shutdown store and try to build state using buildStateForFileCopy assuming state has to be built for the same
// store with 2 blobs present on the partition's file on disk.
store.shutdown();
storageManager.buildStateForFileCopy(newPartition.getReplicaIds().get(0));
dm = storageManager.getDiskManager(newPartition);
store = dm.getStore(newPartition, false);
Expand All @@ -340,28 +347,30 @@ public void buildStateForFileCopyTest() throws Exception {
* @throws Exception
*/
@Test
public void buildStateForFileCopyFailureToAddBlobStoreTest() throws Exception {
generateConfigs(true, false);
MockDataNodeId localNode = clusterMap.getDataNodes().get(0);
public void buildStateForFileCopyDuplicateBlobStoreFailureTest() throws Exception {
int newMountPathIndex = 3;
// add new MountPath to local node
File f = File.createTempFile("ambry", ".tmp");
File mountFile =
new File(f.getParent(), "mountpathfile" + MockClusterMap.PLAIN_TEXT_PORT_START_NUMBER + newMountPathIndex);
MockClusterMap.deleteFileOrDirectory(mountFile);
assertTrue("Couldn't create mount path directory", mountFile.mkdir());
localNode.addMountPaths(Collections.singletonList(mountFile.getAbsolutePath()));
int newPartitionId = 803;
StorageManager storageManager = initializeStorageManagerForStateBuildTests(newMountPathIndex);

PartitionId newPartition =
new MockPartitionId(newPartitionId, MockClusterMap.DEFAULT_PARTITION_CLASS, clusterMap.getDataNodes(), newMountPathIndex);
StorageManager storageManager = createStorageManager(localNode, metricRegistry, null);
storageManager.start();

// test add store onto a new disk, which should succeed
assertTrue("Add new store should succeed", storageManager.addBlobStore(newPartition.getReplicaIds().get(0)));
assertNotNull("The store shouldn't be null because new store is successfully added",
storageManager.getStore(newPartition, false));
// This should fail since the newPartition already has store started.
// Attempting to add store via addBlobStoreForFileCopy should fail since the newPartition already has store started.
assertFalse("Add store which is already existing should fail", storageManager.addBlobStoreForFileCopy(newPartition.getReplicaIds().get(0)));
storageManager.getStore(newPartition, false).shutdown();

// Testing flow where addBlobStoreForFileCopy is called before addBlobStore
// test add store onto a new disk, which should succeed
assertTrue("Add store using addBlobStoreForFileCopy should succeed", storageManager.addBlobStoreForFileCopy(newPartition.getReplicaIds().get(0)));
assertNotNull("The store shouldn't be null because new store is successfully added",
storageManager.getStore(newPartition, false));
// This should fail since the newPartition already has store started.
assertFalse("Add the duplicate store using addBlobStore should fail", storageManager.addBlobStore(newPartition.getReplicaIds().get(0)));
storageManager.getStore(newPartition, false).shutdown();
}

/**
Expand All @@ -370,21 +379,12 @@ public void buildStateForFileCopyFailureToAddBlobStoreTest() throws Exception {
*/
@Test(expected = StateTransitionException.class)
public void buildStateForFileCopyReplicaNullFailureTest() throws Exception {
generateConfigs(true, false);
MockDataNodeId localNode = clusterMap.getDataNodes().get(0);
int newMountPathIndex = 3;
// add new MountPath to local node
File f = File.createTempFile("ambry", ".tmp");
File mountFile =
new File(f.getParent(), "mountpathfile" + MockClusterMap.PLAIN_TEXT_PORT_START_NUMBER + newMountPathIndex);
MockClusterMap.deleteFileOrDirectory(mountFile);
assertTrue("Couldn't create mount path directory", mountFile.mkdir());
localNode.addMountPaths(Collections.singletonList(mountFile.getAbsolutePath()));
int newPartitionId = 803;
StorageManager storageManager = initializeStorageManagerForStateBuildTests(newMountPathIndex);

PartitionId newPartition =
new MockPartitionId(newPartitionId, MockClusterMap.DEFAULT_PARTITION_CLASS, clusterMap.getDataNodes(), newMountPathIndex);
StorageManager storageManager = createStorageManager(localNode, metricRegistry, null);
storageManager.start();
// test add store onto a new disk, which should succeed
assertTrue("Add new store should succeed", storageManager.addBlobStore(newPartition.getReplicaIds().get(0)));
assertNotNull("The store shouldn't be null because new store is successfully added",
Expand Down

0 comments on commit 1b97cc7

Please sign in to comment.