From 1b97cc7faacb5361424002bbca9d04d706d3b5d7 Mon Sep 17 00:00:00 2001 From: Jai Balani Date: Sat, 21 Dec 2024 04:27:28 +0530 Subject: [PATCH] PR comments changes --- .../com/github/ambry/store/DiskManager.java | 4 +- .../github/ambry/store/StorageManager.java | 16 ++-- .../ambry/store/StorageManagerTest.java | 74 +++++++++---------- 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/ambry-store/src/main/java/com/github/ambry/store/DiskManager.java b/ambry-store/src/main/java/com/github/ambry/store/DiskManager.java index 70bb4d6037..3e6dc6feff 100644 --- a/ambry-store/src/main/java/com/github/ambry/store/DiskManager.java +++ b/ambry-store/src/main/java/com/github/ambry/store/DiskManager.java @@ -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(); diff --git a/ambry-store/src/main/java/com/github/ambry/store/StorageManager.java b/ambry-store/src/main/java/com/github/ambry/store/StorageManager.java index 212c6b731b..99c67a1c9d 100644 --- a/ambry-store/src/main/java/com/github/ambry/store/StorageManager.java +++ b/ambry-store/src/main/java/com/github/ambry/store/StorageManager.java @@ -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; @@ -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); diff --git a/ambry-store/src/test/java/com/github/ambry/store/StorageManagerTest.java b/ambry-store/src/test/java/com/github/ambry/store/StorageManagerTest.java index ea03a769b9..dee00019da 100644 --- a/ambry-store/src/test/java/com/github/ambry/store/StorageManagerTest.java +++ b/ambry-store/src/test/java/com/github/ambry/store/StorageManagerTest.java @@ -294,15 +294,9 @@ 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 = @@ -310,11 +304,24 @@ public void buildStateForFileCopyTest() throws Exception { 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", @@ -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); @@ -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(); } /** @@ -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",