Skip to content

HDDS-13545. MutableVolumeSet code cleanup #9755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 3 commits into from
Feb 12, 2026
Merged

HDDS-13545. MutableVolumeSet code cleanup #9755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5f4094
HDDS-13545. Simplify MutableVolumeSet by removing unused methods and …
ptlrs Feb 12, 2026
0911320
Apply suggestions from code review
ptlrs Feb 12, 2026
4767ccd
Update hadoop-hdds/container-service/src/test/java/org/apache/hadoop/…
ptlrs Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,11 @@ private void startRatisForTest() throws IOException {
MutableVolumeSet volumeSet =
getDatanodeStateMachine().getContainer().getVolumeSet();

Map<String, StorageVolume> volumeMap = volumeSet.getVolumeMap();
List<StorageVolume> volumeList = volumeSet.getVolumesList();

for (Map.Entry<String, StorageVolume> entry : volumeMap.entrySet()) {
HddsVolume hddsVolume = (HddsVolume) entry.getValue();
boolean result = StorageVolumeUtil.checkVolume(hddsVolume, clusterId,
clusterId, conf, LOG, null);
for (StorageVolume storageVolume : volumeList) {
HddsVolume hddsVolume = (HddsVolume) storageVolume;
boolean result = StorageVolumeUtil.checkVolume(hddsVolume, clusterId, clusterId, conf, LOG, null);
if (!result) {
volumeSet.failVolume(hddsVolume.getHddsRootDir().getPath());
}
Expand Down
2 changes: 1 addition & 1 deletion ...in/java/org/apache/hadoop/ozone/container/common/states/endpoint/VersionEndpointTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ private void checkVolumeSet(MutableVolumeSet volumeSet,
try {
// If version file does not exist
// create version file and also set scm ID or cluster ID.
for (StorageVolume volume : volumeSet.getVolumeMap().values()) {
for (StorageVolume volume : volumeSet.getVolumesList()) {
boolean result = StorageVolumeUtil.checkVolume(volume,
scmId, clusterId, configuration, LOG,
ozoneContainer.getDbVolumeSet());
Expand Down
96 changes: 5 additions & 91 deletions ...rvice/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,13 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Function;
import org.apache.hadoop.fs.StorageType;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.fs.SpaceUsageCheckFactory;
import org.apache.hadoop.hdds.utils.HddsServerUtil;
Expand Down Expand Up @@ -64,15 +61,9 @@ public class MutableVolumeSet implements VolumeSet {
*/
private Map<String, StorageVolume> failedVolumeMap;

/**
* Maintains a list of active volumes per StorageType.
*/
private EnumMap<StorageType, List<StorageVolume>> volumeStateMap;

/**
* A Reentrant Read Write Lock to synchronize volume operations in VolumeSet.
* Any update to {@link #volumeMap}, {@link #failedVolumeMap}, or
* {@link #volumeStateMap} should be done after acquiring the write lock.
* Any update to {@link #volumeMap} or {@link #failedVolumeMap} should be done after acquiring the write lock.
*/
private final ReentrantReadWriteLock volumeSetRWLock;

Expand Down Expand Up @@ -149,7 +140,6 @@ public StorageVolumeChecker getVolumeChecker() {
private void initializeVolumeSet() throws IOException {
volumeMap = new ConcurrentHashMap<>();
failedVolumeMap = new ConcurrentHashMap<>();
volumeStateMap = new EnumMap<>(StorageType.class);

Collection<String> rawLocations;
if (volumeType == StorageVolume.VolumeType.META_VOLUME) {
Expand All @@ -160,10 +150,6 @@ private void initializeVolumeSet() throws IOException {
rawLocations = HddsServerUtil.getDatanodeStorageDirs(conf);
}

for (StorageType storageType : StorageType.values()) {
volumeStateMap.put(storageType, new ArrayList<>());
}

for (String locationString : rawLocations) {
StorageVolume volume = null;
try {
Expand All @@ -181,7 +167,6 @@ private void initializeVolumeSet() throws IOException {
volume.getStorageDir());
}
volumeMap.put(volume.getStorageDir().getPath(), volume);
volumeStateMap.get(volume.getStorageType()).add(volume);
volumeHealthMetrics.incrementHealthyVolumes();
} catch (IOException e) {
volumeHealthMetrics.incrementFailedVolumes();
Expand Down Expand Up @@ -330,45 +315,6 @@ public void writeUnlock() {
volumeSetRWLock.writeLock().unlock();
}

// Add a volume to VolumeSet
boolean addVolume(String dataDir) {
return addVolume(dataDir, StorageType.DEFAULT);
}

// Add a volume to VolumeSet
private boolean addVolume(String volumeRoot, StorageType storageType) {
boolean success;

this.writeLock();
try {
if (volumeMap.containsKey(volumeRoot)) {
LOG.warn("Volume : {} already exists in VolumeMap", volumeRoot);
success = false;
} else {
if (failedVolumeMap.containsKey(volumeRoot)) {
failedVolumeMap.remove(volumeRoot);
volumeHealthMetrics.decrementFailedVolumes();
}

StorageVolume volume =
volumeFactory.createVolume(volumeRoot, storageType);
volumeMap.put(volume.getStorageDir().getPath(), volume);
volumeStateMap.get(volume.getStorageType()).add(volume);

LOG.info("Added Volume : {} to VolumeSet",
volume.getStorageDir().getPath());
success = true;
volumeHealthMetrics.incrementHealthyVolumes();
}
} catch (IOException ex) {
LOG.error("Failed to add volume " + volumeRoot + " to VolumeSet", ex);
success = false;
} finally {
this.writeUnlock();
}
return success;
}

// Mark a volume as failed
public void failVolume(String volumeRoot) {
this.writeLock();
Expand All @@ -378,7 +324,6 @@ public void failVolume(String volumeRoot) {
volume.failVolume();

volumeMap.remove(volumeRoot);
volumeStateMap.get(volume.getStorageType()).remove(volume);
failedVolumeMap.put(volumeRoot, volume);
volumeHealthMetrics.decrementHealthyVolumes();
volumeHealthMetrics.incrementFailedVolumes();
Expand All @@ -393,30 +338,6 @@ public void failVolume(String volumeRoot) {
}
}

// Remove a volume from the VolumeSet completely.
public void removeVolume(String volumeRoot) throws IOException {
this.writeLock();
try {
if (volumeMap.containsKey(volumeRoot)) {
StorageVolume volume = volumeMap.get(volumeRoot);
volume.shutdown();

volumeMap.remove(volumeRoot);
volumeStateMap.get(volume.getStorageType()).remove(volume);
volumeHealthMetrics.decrementHealthyVolumes();
LOG.info("Removed Volume : {} from VolumeSet", volumeRoot);
} else if (failedVolumeMap.containsKey(volumeRoot)) {
failedVolumeMap.remove(volumeRoot);
volumeHealthMetrics.decrementFailedVolumes();
LOG.info("Removed Volume : {} from failed VolumeSet", volumeRoot);
} else {
LOG.warn("Volume : {} does not exist in VolumeSet", volumeRoot);
}
} finally {
this.writeUnlock();
}
}

/**
* Shutdown the volumeset.
*/
Expand All @@ -436,7 +357,6 @@ public void shutdown() {
}

@Override
@VisibleForTesting
public List<StorageVolume> getVolumesList() {
return ImmutableList.copyOf(volumeMap.values());
}
Expand All @@ -456,26 +376,20 @@ public void setVolumeMap(Map<String, StorageVolume> map) {
this.volumeMap = map;
}

@VisibleForTesting
public Map<StorageType, List<StorageVolume>> getVolumeStateMap() {
return ImmutableMap.copyOf(volumeStateMap);
}

public boolean hasEnoughVolumes() {
// Max number of bad volumes allowed, should have at least
// 1 good volume
boolean hasEnoughVolumes;
if (maxVolumeFailuresTolerated ==
StorageVolumeChecker.MAX_VOLUME_FAILURE_TOLERATED_LIMIT) {
hasEnoughVolumes = !getVolumesList().isEmpty();
if (maxVolumeFailuresTolerated == StorageVolumeChecker.MAX_VOLUME_FAILURE_TOLERATED_LIMIT) {
hasEnoughVolumes = !volumeMap.isEmpty();
} else {
hasEnoughVolumes = getFailedVolumesList().size() <= maxVolumeFailuresTolerated;
hasEnoughVolumes = failedVolumeMap.size() <= maxVolumeFailuresTolerated;
}
if (!hasEnoughVolumes) {
LOG.error("Not enough volumes in MutableVolumeSet. DatanodeUUID: {}, VolumeType: {}, " +
"MaxVolumeFailuresTolerated: {}, ActiveVolumes: {}, FailedVolumes: {}",
datanodeUuid, volumeType, maxVolumeFailuresTolerated,
getVolumesList().size(), getFailedVolumesList().size());
volumeMap.size(), failedVolumeMap.size());
}
return hasEnoughVolumes;
}
Expand Down
8 changes: 5 additions & 3 deletions ...c/test/java/org/apache/hadoop/ozone/container/common/volume/TestStorageVolumeChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ public void testVolumeDeletion(VolumeCheckResult checkResult,
conf.getObject(DatanodeConfiguration.class);
dnConf.setDiskCheckMinGap(Duration.ofMillis(0));
conf.setFromObject(dnConf);
File volParentDir =
new File(folder.toString(), UUID.randomUUID().toString());
conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_KEY,
conf.get(ScmConfigKeys.HDDS_DATANODE_DIR_KEY) + "," + volParentDir.getPath());

DatanodeDetails datanodeDetails =
ContainerTestUtils.createDatanodeDetails();
Expand All @@ -218,9 +222,7 @@ public void testVolumeDeletion(VolumeCheckResult checkResult,

StorageVolumeChecker volumeChecker = volumeSet.getVolumeChecker();
volumeChecker.setDelegateChecker(new DummyChecker());
File volParentDir =
new File(folder.toString(), UUID.randomUUID().toString());
volumeSet.addVolume(volParentDir.getPath());

File volRootDir = new File(volParentDir, "hdds");

int i = 0;
Expand Down
52 changes: 5 additions & 47 deletions ...-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@
import org.apache.commons.io.FileUtils;
import org.apache.hadoop.hdds.HddsConfigKeys;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.ScmConfigKeys;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
import org.apache.ozone.test.GenericTestUtils.LogCapturer;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -127,25 +127,6 @@ public void testVolumeSetInitialization() throws Exception {
assertNumVolumes(volumeSet, 2, 0);
}

@Test
public void testAddVolume() {

assertEquals(2, volumeSet.getVolumesList().size());

assertNumVolumes(volumeSet, 2, 0);

// Add a volume to VolumeSet
String volume3 = baseDir.resolve("disk3").toString();
boolean success = volumeSet.addVolume(volume3);

assertTrue(success);
assertEquals(3, volumeSet.getVolumesList().size());
assertTrue(checkVolumeExistsInVolumeSet(volume3),
"AddVolume did not add requested volume to VolumeSet");

assertNumVolumes(volumeSet, 3, 0);
}

@Test
public void testFailVolume() throws Exception {
assertNumVolumes(volumeSet, 2, 0);
Expand All @@ -169,30 +150,6 @@ public void testFailVolume() throws Exception {
assertNumVolumes(volumeSet, 1, 1);
}

@Test
public void testRemoveVolume() throws Exception {
assertNumVolumes(volumeSet, 2, 0);

assertEquals(2, volumeSet.getVolumesList().size());

// Remove a volume from VolumeSet
volumeSet.removeVolume(HddsVolumeUtil.getHddsRoot(volume1));
assertEquals(1, volumeSet.getVolumesList().size());

assertNumVolumes(volumeSet, 1, 0);

// Attempting to remove a volume which does not exist in VolumeSet should
// log a warning.
LogCapturer logs = LogCapturer.captureLogs(MutableVolumeSet.class);
volumeSet.removeVolume(HddsVolumeUtil.getHddsRoot(volume1));
assertEquals(1, volumeSet.getVolumesList().size());
String expectedLogMessage = "Volume : " +
HddsVolumeUtil.getHddsRoot(volume1) + " does not exist in VolumeSet";
assertThat(logs.getOutput()).contains(expectedLogMessage);

assertNumVolumes(volumeSet, 1, 0);
}

@Test
public void testVolumeInInconsistentState() throws Exception {
assertNumVolumes(volumeSet, 2, 0);
Expand All @@ -213,14 +170,15 @@ public void testVolumeInInconsistentState() throws Exception {
// The new volume is in an inconsistent state as the root dir is
// non-empty but the version file does not exist. Add Volume should
// return false.
Comment on lines 170 to 172
Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is outdated now after the changes, so needs to be updated.

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why comments should not duplicate statements (assertFalse in this case)... :)

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this for follow-up, since it's a test-only change. There can be further cleanup in the test class.

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created HDDS-14627.

Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

boolean success = volumeSet.addVolume(volume3);
conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_KEY, conf.get(ScmConfigKeys.HDDS_DATANODE_DIR_KEY) + "," + volume3);
volumeSet.shutdown();
initializeVolumeSet();

assertFalse(success);
assertEquals(2, volumeSet.getVolumesList().size());
assertFalse(checkVolumeExistsInVolumeSet(volume3), "AddVolume should fail" +
" for an inconsistent volume");

assertNumVolumes(volumeSet, 2, 0);
assertNumVolumes(volumeSet, 2, 1);
// Delete volume3
File volume = new File(volume3);
FileUtils.deleteDirectory(volume);
Expand Down