From fc9a47fa91a1774f08a0b98621d9da8a461f7e7e Mon Sep 17 00:00:00 2001 From: sarvekshayr Date: Mon, 13 Oct 2025 15:53:56 +0530 Subject: [PATCH 1/4] HDDS-13639. Optimize container iterator for frequent operation --- .../container/common/impl/ContainerSet.java | 95 ++++++++++++++-- .../common/impl/TestContainerSet.java | 103 ++++++++++++++++++ 2 files changed, 191 insertions(+), 7 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java index baf6d48a9492..5ae69ff1adf3 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java @@ -34,12 +34,15 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.ToLongFunction; +import java.util.stream.Collectors; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReportsProto; @@ -75,6 +78,14 @@ public class ContainerSet implements Iterable> { private final WitnessedContainerMetadataStore containerMetadataStore; // Handler that will be invoked when a scan of a container in this set is requested. private OnDemandContainerScanner containerScanner; + + // Maps volume storage ID to a set of container IDs + private final ConcurrentHashMap> volumeToContainersMap = + new ConcurrentHashMap<>(); + + // Maps volume storage ID to container count + private final ConcurrentHashMap volumeContainerCountCache = + new ConcurrentHashMap<>(); public static ContainerSet newReadOnlyContainerSet(long recoveringTimeout) { return new ContainerSet(null, recoveringTimeout); @@ -205,6 +216,9 @@ private boolean addContainer(Container container, boolean overwrite) throws recoveringContainerMap.put( clock.millis() + recoveringTimeout, containerId); } + // Update per-volume index and count cache + updateVolumeIndexOnAdd(container); + return true; } else { LOG.warn("Container already exists with container Id {}", containerId); @@ -213,6 +227,29 @@ private boolean addContainer(Container container, boolean overwrite) throws ContainerProtos.Result.CONTAINER_EXISTS); } } + + /** + * Updates the per-volume container index when a container is added. + * + * @param container the container being added + */ + private void updateVolumeIndexOnAdd(Container container) { + HddsVolume volume = container.getContainerData().getVolume(); + if (volume != null && volume.getStorageID() != null) { + String volumeUuid = volume.getStorageID(); + long containerId = container.getContainerData().getContainerID(); + + // Add container ID to volume's container set + volumeToContainersMap + .computeIfAbsent(volumeUuid, k -> new ConcurrentSkipListSet<>()) + .add(containerId); + + // Increment volume's container count + volumeContainerCountCache + .computeIfAbsent(volumeUuid, k -> new AtomicLong(0)) + .incrementAndGet(); + } + } private void updateContainerIdTable(long containerId, ContainerData containerData) throws StorageContainerException { if (null != containerMetadataStore) { @@ -299,11 +336,49 @@ private boolean removeContainer(long containerId, boolean markMissing, boolean r "containerMap", containerId); return false; } else { + // Update per-volume index and count cache + updateVolumeIndexOnRemove(removed); + LOG.debug("Container with containerId {} is removed from containerMap", containerId); return true; } } + + /** + * Updates the per-volume container index when a container is removed. + * + * @param container the container being removed + */ + private void updateVolumeIndexOnRemove(Container container) { + HddsVolume volume = container.getContainerData().getVolume(); + if (volume != null && volume.getStorageID() != null) { + String volumeUuid = volume.getStorageID(); + long containerId = container.getContainerData().getContainerID(); + + // Remove container ID from volume's container set + ConcurrentSkipListSet containerSet = volumeToContainersMap.get(volumeUuid); + if (containerSet != null) { + containerSet.remove(containerId); + + // If the set is now empty, remove it from the map to save memory + if (containerSet.isEmpty()) { + volumeToContainersMap.remove(volumeUuid); + } + } + + // Decrement volume's container count + AtomicLong count = volumeContainerCountCache.get(volumeUuid); + if (count != null) { + long newCount = count.decrementAndGet(); + + // If count reaches zero, remove from cache to save memory + if (newCount <= 0) { + volumeContainerCountCache.remove(volumeUuid); + } + } + } + } private void deleteFromContainerTable(long containerId) throws StorageContainerException { if (null != containerMetadataStore) { @@ -411,11 +486,18 @@ public Iterator> getContainerIterator(HddsVolume volume) { Preconditions.checkNotNull(volume); Preconditions.checkNotNull(volume.getStorageID()); String volumeUuid = volume.getStorageID(); - return containerMap.values().stream() - .filter(x -> volumeUuid.equals(x.getContainerData().getVolume() - .getStorageID())) + + ConcurrentSkipListSet containerIds = volumeToContainersMap.get(volumeUuid); + if (containerIds == null || containerIds.isEmpty()) { + return Collections.emptyIterator(); + } + List> containers = containerIds.stream() + .map(containerMap::get) + .filter(Objects::nonNull) .sorted(ContainerDataScanOrder.INSTANCE) - .iterator(); + .collect(Collectors.toList()); + + return containers.iterator(); } /** @@ -428,9 +510,8 @@ public long containerCount(HddsVolume volume) { Preconditions.checkNotNull(volume); Preconditions.checkNotNull(volume.getStorageID()); String volumeUuid = volume.getStorageID(); - return containerMap.values().stream() - .filter(x -> volumeUuid.equals(x.getContainerData().getVolume() - .getStorageID())).count(); + AtomicLong count = volumeContainerCountCache.get(volumeUuid); + return count != null ? count.get() : 0; } /** diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java index 8c54dd848af4..d84ef715c4d6 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java @@ -375,4 +375,107 @@ private ContainerSet createContainerSet() throws StorageContainerException { return containerSet; } + /** + * Test that containerCount per volume returns correct count. + */ + @ContainerLayoutTestInfo.ContainerTest + public void testContainerCountPerVolume(ContainerLayoutVersion layout) + throws StorageContainerException { + setLayoutVersion(layout); + HddsVolume vol1 = mock(HddsVolume.class); + when(vol1.getStorageID()).thenReturn("uuid-1"); + HddsVolume vol2 = mock(HddsVolume.class); + when(vol2.getStorageID()).thenReturn("uuid-2"); + HddsVolume vol3 = mock(HddsVolume.class); + when(vol3.getStorageID()).thenReturn("uuid-3"); + + ContainerSet containerSet = newContainerSet(); + + // Add 100 containers to vol1, 50 to vol2, 0 to vol3 + for (int i = 0; i < 100; i++) { + KeyValueContainerData kvData = new KeyValueContainerData(i, + layout, + (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(), + UUID.randomUUID().toString()); + kvData.setVolume(vol1); + kvData.setState(ContainerProtos.ContainerDataProto.State.CLOSED); + containerSet.addContainer(new KeyValueContainer(kvData, new OzoneConfiguration())); + } + + for (int i = 100; i < 150; i++) { + KeyValueContainerData kvData = new KeyValueContainerData(i, + layout, + (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(), + UUID.randomUUID().toString()); + kvData.setVolume(vol2); + kvData.setState(ContainerProtos.ContainerDataProto.State.CLOSED); + containerSet.addContainer(new KeyValueContainer(kvData, new OzoneConfiguration())); + } + + // Verify counts + assertEquals(100, containerSet.containerCount(vol1)); + assertEquals(50, containerSet.containerCount(vol2)); + assertEquals(0, containerSet.containerCount(vol3)); + + // Remove some containers and verify counts are updated + containerSet.removeContainer(0); + containerSet.removeContainer(1); + containerSet.removeContainer(100); + assertEquals(98, containerSet.containerCount(vol1)); + assertEquals(49, containerSet.containerCount(vol2)); + } + + /** + * Test that per-volume iterator only returns containers from that volume. + */ + @ContainerLayoutTestInfo.ContainerTest + public void testContainerIteratorPerVolume(ContainerLayoutVersion layout) + throws StorageContainerException { + setLayoutVersion(layout); + HddsVolume vol1 = mock(HddsVolume.class); + when(vol1.getStorageID()).thenReturn("uuid-11"); + HddsVolume vol2 = mock(HddsVolume.class); + when(vol2.getStorageID()).thenReturn("uuid-12"); + + ContainerSet containerSet = newContainerSet(); + + // Add containers with specific IDs to each volume + List vol1Ids = new ArrayList<>(); + List vol2Ids = new ArrayList<>(); + + for (int i = 0; i < 20; i++) { + KeyValueContainerData kvData = new KeyValueContainerData(i, + layout, + (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(), + UUID.randomUUID().toString()); + if (i % 2 == 0) { + kvData.setVolume(vol1); + vol1Ids.add((long) i); + } else { + kvData.setVolume(vol2); + vol2Ids.add((long) i); + } + kvData.setState(ContainerProtos.ContainerDataProto.State.CLOSED); + containerSet.addContainer(new KeyValueContainer(kvData, new OzoneConfiguration())); + } + + // Verify iterator only returns containers from vol1 + Iterator> iter1 = containerSet.getContainerIterator(vol1); + List foundVol1Ids = new ArrayList<>(); + while (iter1.hasNext()) { + foundVol1Ids.add(iter1.next().getContainerData().getContainerID()); + } + assertEquals(vol1Ids.size(), foundVol1Ids.size()); + assertTrue(foundVol1Ids.containsAll(vol1Ids)); + + // Verify iterator only returns containers from vol2 + Iterator> iter2 = containerSet.getContainerIterator(vol2); + List foundVol2Ids = new ArrayList<>(); + while (iter2.hasNext()) { + foundVol2Ids.add(iter2.next().getContainerData().getContainerID()); + } + assertEquals(vol2Ids.size(), foundVol2Ids.size()); + assertTrue(foundVol2Ids.containsAll(vol2Ids)); + } + } From 8fd59ff15d68a883d36944d55e7edae332e640c8 Mon Sep 17 00:00:00 2001 From: sarvekshayr Date: Mon, 20 Oct 2025 21:56:24 +0530 Subject: [PATCH 2/4] Track containers in HddsVolume --- .../container/common/impl/ContainerSet.java | 111 ++++-------------- .../container/common/volume/HddsVolume.java | 20 ++++ .../common/impl/TestContainerSet.java | 53 ++++++--- 3 files changed, 77 insertions(+), 107 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java index 5ae69ff1adf3..97b958d42e5b 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java @@ -34,15 +34,12 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.ToLongFunction; -import java.util.stream.Collectors; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReportsProto; @@ -78,14 +75,6 @@ public class ContainerSet implements Iterable> { private final WitnessedContainerMetadataStore containerMetadataStore; // Handler that will be invoked when a scan of a container in this set is requested. private OnDemandContainerScanner containerScanner; - - // Maps volume storage ID to a set of container IDs - private final ConcurrentHashMap> volumeToContainersMap = - new ConcurrentHashMap<>(); - - // Maps volume storage ID to container count - private final ConcurrentHashMap volumeContainerCountCache = - new ConcurrentHashMap<>(); public static ContainerSet newReadOnlyContainerSet(long recoveringTimeout) { return new ContainerSet(null, recoveringTimeout); @@ -216,9 +205,10 @@ private boolean addContainer(Container container, boolean overwrite) throws recoveringContainerMap.put( clock.millis() + recoveringTimeout, containerId); } - // Update per-volume index and count cache - updateVolumeIndexOnAdd(container); - + HddsVolume volume = container.getContainerData().getVolume(); + if (volume != null) { + volume.addContainer(containerId); + } return true; } else { LOG.warn("Container already exists with container Id {}", containerId); @@ -227,29 +217,6 @@ private boolean addContainer(Container container, boolean overwrite) throws ContainerProtos.Result.CONTAINER_EXISTS); } } - - /** - * Updates the per-volume container index when a container is added. - * - * @param container the container being added - */ - private void updateVolumeIndexOnAdd(Container container) { - HddsVolume volume = container.getContainerData().getVolume(); - if (volume != null && volume.getStorageID() != null) { - String volumeUuid = volume.getStorageID(); - long containerId = container.getContainerData().getContainerID(); - - // Add container ID to volume's container set - volumeToContainersMap - .computeIfAbsent(volumeUuid, k -> new ConcurrentSkipListSet<>()) - .add(containerId); - - // Increment volume's container count - volumeContainerCountCache - .computeIfAbsent(volumeUuid, k -> new AtomicLong(0)) - .incrementAndGet(); - } - } private void updateContainerIdTable(long containerId, ContainerData containerData) throws StorageContainerException { if (null != containerMetadataStore) { @@ -336,49 +303,15 @@ private boolean removeContainer(long containerId, boolean markMissing, boolean r "containerMap", containerId); return false; } else { - // Update per-volume index and count cache - updateVolumeIndexOnRemove(removed); - + HddsVolume volume = removed.getContainerData().getVolume(); + if (volume != null) { + volume.removeContainer(containerId); + } LOG.debug("Container with containerId {} is removed from containerMap", containerId); return true; } } - - /** - * Updates the per-volume container index when a container is removed. - * - * @param container the container being removed - */ - private void updateVolumeIndexOnRemove(Container container) { - HddsVolume volume = container.getContainerData().getVolume(); - if (volume != null && volume.getStorageID() != null) { - String volumeUuid = volume.getStorageID(); - long containerId = container.getContainerData().getContainerID(); - - // Remove container ID from volume's container set - ConcurrentSkipListSet containerSet = volumeToContainersMap.get(volumeUuid); - if (containerSet != null) { - containerSet.remove(containerId); - - // If the set is now empty, remove it from the map to save memory - if (containerSet.isEmpty()) { - volumeToContainersMap.remove(volumeUuid); - } - } - - // Decrement volume's container count - AtomicLong count = volumeContainerCountCache.get(volumeUuid); - if (count != null) { - long newCount = count.decrementAndGet(); - - // If count reaches zero, remove from cache to save memory - if (newCount <= 0) { - volumeContainerCountCache.remove(volumeUuid); - } - } - } - } private void deleteFromContainerTable(long containerId) throws StorageContainerException { if (null != containerMetadataStore) { @@ -484,19 +417,18 @@ public Iterator> getRecoveringContainerIterator() { */ public Iterator> getContainerIterator(HddsVolume volume) { Preconditions.checkNotNull(volume); - Preconditions.checkNotNull(volume.getStorageID()); - String volumeUuid = volume.getStorageID(); - - ConcurrentSkipListSet containerIds = volumeToContainersMap.get(volumeUuid); - if (containerIds == null || containerIds.isEmpty()) { - return Collections.emptyIterator(); + Iterator containerIdIterator = volume.getContainerIterator(); + + List> containers = new ArrayList<>(); + while (containerIdIterator.hasNext()) { + Long containerId = containerIdIterator.next(); + Container container = containerMap.get(containerId); + if (container != null) { + containers.add(container); + } } - List> containers = containerIds.stream() - .map(containerMap::get) - .filter(Objects::nonNull) - .sorted(ContainerDataScanOrder.INSTANCE) - .collect(Collectors.toList()); - + containers.sort(ContainerDataScanOrder.INSTANCE); + return containers.iterator(); } @@ -508,10 +440,7 @@ public Iterator> getContainerIterator(HddsVolume volume) { */ public long containerCount(HddsVolume volume) { Preconditions.checkNotNull(volume); - Preconditions.checkNotNull(volume.getStorageID()); - String volumeUuid = volume.getStorageID(); - AtomicLong count = volumeContainerCountCache.get(volumeUuid); - return count != null ? count.get() : 0; + return volume.getContainerCount(); } /** diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java index d6f404dd17ea..0988064e5fe8 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java @@ -25,9 +25,11 @@ import jakarta.annotation.Nullable; import java.io.File; import java.io.IOException; +import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Queue; +import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -93,6 +95,8 @@ public class HddsVolume extends StorageVolume { private final AtomicLong committedBytes = new AtomicLong(); // till Open containers become full private Function gatherContainerUsages = (K) -> 0L; + private final ConcurrentSkipListSet containerIds = new ConcurrentSkipListSet<>(); + // Mentions the type of volume private final VolumeType type = VolumeType.DATA_VOLUME; // The dedicated DbVolume that the db instance of this HddsVolume resides. @@ -529,6 +533,22 @@ public long getContainers() { return 0; } + public void addContainer(long containerId) { + containerIds.add(containerId); + } + + public void removeContainer(long containerId) { + containerIds.remove(containerId); + } + + public Iterator getContainerIterator() { + return containerIds.iterator(); + } + + public long getContainerCount() { + return containerIds.size(); + } + /** * Pick a DbVolume for HddsVolume and init db instance. * Use the HddsVolume directly if no DbVolume found. diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java index d84ef715c4d6..efb4be86e8dc 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java @@ -28,6 +28,7 @@ import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -40,6 +41,7 @@ import java.util.Optional; import java.util.Random; import java.util.UUID; +import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.LongStream; import org.apache.hadoop.conf.StorageUnit; @@ -69,6 +71,33 @@ private void setLayoutVersion(ContainerLayoutVersion layoutVersion) { this.layoutVersion = layoutVersion; } + /** + * Create a mock {@link HddsVolume} to track container IDs. + */ + private HddsVolume mockHddsVolume(String storageId) { + HddsVolume volume = mock(HddsVolume.class); + when(volume.getStorageID()).thenReturn(storageId); + + ConcurrentSkipListSet containerIds = new ConcurrentSkipListSet<>(); + + doAnswer(inv -> { + Long containerId = inv.getArgument(0); + containerIds.add(containerId); + return null; + }).when(volume).addContainer(any(Long.class)); + + doAnswer(inv -> { + Long containerId = inv.getArgument(0); + containerIds.remove(containerId); + return null; + }).when(volume).removeContainer(any(Long.class)); + + when(volume.getContainerIterator()).thenAnswer(inv -> containerIds.iterator()); + when(volume.getContainerCount()).thenAnswer(inv -> (long) containerIds.size()); + + return volume; + } + @ContainerLayoutTestInfo.ContainerTest public void testAddGetRemoveContainer(ContainerLayoutVersion layout) throws StorageContainerException { @@ -157,10 +186,8 @@ public void testIteratorsAndCount(ContainerLayoutVersion layout) public void testIteratorPerVolume(ContainerLayoutVersion layout) throws StorageContainerException { setLayoutVersion(layout); - HddsVolume vol1 = mock(HddsVolume.class); - when(vol1.getStorageID()).thenReturn("uuid-1"); - HddsVolume vol2 = mock(HddsVolume.class); - when(vol2.getStorageID()).thenReturn("uuid-2"); + HddsVolume vol1 = mockHddsVolume("uuid-1"); + HddsVolume vol2 = mockHddsVolume("uuid-2"); ContainerSet containerSet = newContainerSet(); for (int i = 0; i < 10; i++) { @@ -202,8 +229,7 @@ public void testIteratorPerVolume(ContainerLayoutVersion layout) public void iteratorIsOrderedByScanTime(ContainerLayoutVersion layout) throws StorageContainerException { setLayoutVersion(layout); - HddsVolume vol = mock(HddsVolume.class); - when(vol.getStorageID()).thenReturn("uuid-1"); + HddsVolume vol = mockHddsVolume("uuid-1"); Random random = new Random(); ContainerSet containerSet = newContainerSet(); int containerCount = 50; @@ -382,12 +408,9 @@ private ContainerSet createContainerSet() throws StorageContainerException { public void testContainerCountPerVolume(ContainerLayoutVersion layout) throws StorageContainerException { setLayoutVersion(layout); - HddsVolume vol1 = mock(HddsVolume.class); - when(vol1.getStorageID()).thenReturn("uuid-1"); - HddsVolume vol2 = mock(HddsVolume.class); - when(vol2.getStorageID()).thenReturn("uuid-2"); - HddsVolume vol3 = mock(HddsVolume.class); - when(vol3.getStorageID()).thenReturn("uuid-3"); + HddsVolume vol1 = mockHddsVolume("uuid-1"); + HddsVolume vol2 = mockHddsVolume("uuid-2"); + HddsVolume vol3 = mockHddsVolume("uuid-3"); ContainerSet containerSet = newContainerSet(); @@ -432,10 +455,8 @@ public void testContainerCountPerVolume(ContainerLayoutVersion layout) public void testContainerIteratorPerVolume(ContainerLayoutVersion layout) throws StorageContainerException { setLayoutVersion(layout); - HddsVolume vol1 = mock(HddsVolume.class); - when(vol1.getStorageID()).thenReturn("uuid-11"); - HddsVolume vol2 = mock(HddsVolume.class); - when(vol2.getStorageID()).thenReturn("uuid-12"); + HddsVolume vol1 = mockHddsVolume("uuid-11"); + HddsVolume vol2 = mockHddsVolume("uuid-12"); ContainerSet containerSet = newContainerSet(); From 6123c9c39c096adbfab89fd97b5c32ff794e3252 Mon Sep 17 00:00:00 2001 From: sarvekshayr Date: Tue, 21 Oct 2025 09:58:44 +0530 Subject: [PATCH 3/4] Fixed TestDeleteBlocksCommandHandler failures --- .../TestDeleteBlocksCommandHandler.java | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java index b2e1c72e487a..cf216d53c9e6 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java @@ -49,6 +49,7 @@ import java.util.Map; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -104,13 +105,37 @@ private void prepareTest(ContainerTestVersionInfo versionInfo) setup(); } + /** + * Create a mock {@link HddsVolume} to track container IDs. + */ + private HddsVolume mockHddsVolume(String storageId) { + HddsVolume volume = mock(HddsVolume.class); + when(volume.getStorageID()).thenReturn(storageId); + + ConcurrentSkipListSet containerIds = new ConcurrentSkipListSet<>(); + + doAnswer(inv -> { + Long containerId = inv.getArgument(0); + containerIds.add(containerId); + return null; + }).when(volume).addContainer(any(Long.class)); + + doAnswer(inv -> { + Long containerId = inv.getArgument(0); + containerIds.remove(containerId); + return null; + }).when(volume).removeContainer(any(Long.class)); + + when(volume.getContainerIterator()).thenAnswer(inv -> containerIds.iterator()); + return volume; + } + private void setup() throws Exception { OzoneConfiguration conf = new OzoneConfiguration(); ContainerLayoutVersion layout = ContainerLayoutVersion.FILE_PER_BLOCK; OzoneContainer ozoneContainer = mock(OzoneContainer.class); containerSet = newContainerSet(); - volume1 = mock(HddsVolume.class); - when(volume1.getStorageID()).thenReturn("uuid-1"); + volume1 = mockHddsVolume("uuid-1"); for (int i = 0; i <= 10; i++) { KeyValueContainerData data = new KeyValueContainerData(i, From 044183f8d738e9f0f2a5e1b0139d47d92f2fa90f Mon Sep 17 00:00:00 2001 From: sarvekshayr Date: Wed, 22 Oct 2025 11:56:31 +0530 Subject: [PATCH 4/4] Use container iterator in gatherContainerUsages and fix tests --- .../container/ozoneimpl/OzoneContainer.java | 10 +++-- .../TestDeleteBlocksCommandHandler.java | 6 --- .../ozoneimpl/TestOzoneContainer.java | 44 ++++++++++++++++++- 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java index e0f255d64058..47136a51a0aa 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java @@ -591,11 +591,13 @@ public ContainerSet getContainerSet() { public Long gatherContainerUsages(HddsVolume storageVolume) { AtomicLong usages = new AtomicLong(); - containerSet.getContainerMapIterator().forEachRemaining(e -> { - if (e.getValue().getContainerData().getVolume().getStorageID().equals(storageVolume.getStorageID())) { - usages.addAndGet(e.getValue().getContainerData().getBytesUsed()); + Iterator containerIdIterator = storageVolume.getContainerIterator(); + while (containerIdIterator.hasNext()) { + Container container = containerSet.getContainer(containerIdIterator.next()); + if (container != null) { + usages.addAndGet(container.getContainerData().getBytesUsed()); } - }); + } return usages.get(); } /** diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java index cf216d53c9e6..50b08c7aa2f2 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java @@ -120,12 +120,6 @@ private HddsVolume mockHddsVolume(String storageId) { return null; }).when(volume).addContainer(any(Long.class)); - doAnswer(inv -> { - Long containerId = inv.getArgument(0); - containerIds.remove(containerId); - return null; - }).when(volume).removeContainer(any(Long.class)); - when(volume.getContainerIterator()).thenAnswer(inv -> containerIds.iterator()); return volume; } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java index 4cca1dd21cd0..91c3f8ed58c2 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java @@ -21,6 +21,10 @@ import static org.apache.hadoop.ozone.container.common.ContainerTestUtils.createDbInstancesForTestIfNeeded; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.base.Preconditions; import java.io.File; @@ -30,9 +34,11 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Random; import java.util.Set; import java.util.UUID; +import java.util.concurrent.ConcurrentSkipListSet; import org.apache.commons.io.FileUtils; import org.apache.hadoop.conf.StorageUnit; import org.apache.hadoop.hdds.HddsConfigKeys; @@ -110,6 +116,25 @@ public void cleanUp() { } } + /** + * Create a mock {@link HddsVolume} to track container IDs. + */ + private HddsVolume mockHddsVolume(String storageId) { + HddsVolume volume = mock(HddsVolume.class); + when(volume.getStorageID()).thenReturn(storageId); + + ConcurrentSkipListSet containerIds = new ConcurrentSkipListSet<>(); + + doAnswer(inv -> { + Long containerId = inv.getArgument(0); + containerIds.add(containerId); + return null; + }).when(volume).addContainer(any(Long.class)); + + when(volume.getContainerIterator()).thenAnswer(inv -> containerIds.iterator()); + return volume; + } + @ContainerTestVersionInfo.ContainerTest public void testBuildContainerMap(ContainerTestVersionInfo versionInfo) throws Exception { @@ -117,9 +142,14 @@ public void testBuildContainerMap(ContainerTestVersionInfo versionInfo) // Format the volumes List volumes = StorageVolumeUtil.getHddsVolumesList(volumeSet.getVolumesList()); + + // Create mock volumes with tracking, mapped by storage ID + Map mockVolumeMap = new HashMap<>(); for (HddsVolume volume : volumes) { volume.format(clusterId); commitSpaceMap.put(getVolumeKey(volume), Long.valueOf(0)); + // Create mock for each real volume + mockVolumeMap.put(volume.getStorageID(), mockHddsVolume(volume.getStorageID())); } List containerDatas = new ArrayList<>(); // Add containers to disk @@ -140,6 +170,12 @@ public void testBuildContainerMap(ContainerTestVersionInfo versionInfo) keyValueContainerData, conf); keyValueContainer.create(volumeSet, volumeChoosingPolicy, clusterId); myVolume = keyValueContainer.getContainerData().getVolume(); + + // Track container in mock volume + HddsVolume mockVolume = mockVolumeMap.get(myVolume.getStorageID()); + if (mockVolume != null) { + mockVolume.addContainer(i); + } freeBytes = addBlocks(keyValueContainer, 2, 3, 65536); @@ -158,7 +194,13 @@ public void testBuildContainerMap(ContainerTestVersionInfo versionInfo) assertEquals(numTestContainers, containerset.containerCount()); verifyCommittedSpace(ozoneContainer); // container usage here, nrOfContainer * blocks * chunksPerBlock * datalen - assertEquals(10 * 2 * 3 * 65536, ozoneContainer.gatherContainerUsages(volumes.get(0))); + // Use mock volumes to verify container usage + long totalUsage = 0; + for (HddsVolume volume : volumes) { + HddsVolume mockVolume = mockVolumeMap.get(volume.getStorageID()); + totalUsage += ozoneContainer.gatherContainerUsages(mockVolume); + } + assertEquals(10 * 2 * 3 * 65536, totalUsage); Set missingContainers = new HashSet<>(); for (int i = 0; i < numTestContainers; i++) { if (i % 2 == 0) {