Skip to content
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

HDDS-12547. Container creation and import use the same VolumeChoosingPolicy #8090

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

HDDS-12547. Container creation and import use the same VolumeChoosingPolicy #8090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f3a844
HDDS-12547. Container creation and import use the same VolumeChoosing…
peterxcli Mar 15, 2025
8d1e90a
Don't handle exception in VolumeChoosingPolicyFactory#getSharedPolicy
peterxcli Mar 15, 2025
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
6 changes: 2 additions & 4 deletions ...in/java/org/apache/hadoop/ozone/container/common/volume/CapacityVolumeChoosingPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

import java.io.IOException;
import java.util.List;
import java.util.Random;
import java.util.concurrent.ThreadLocalRandom;
import java.util.stream.Collectors;
import org.apache.hadoop.ozone.container.common.interfaces.VolumeChoosingPolicy;
import org.apache.hadoop.util.DiskChecker.DiskOutOfSpaceException;
Expand All @@ -44,9 +44,6 @@ public class CapacityVolumeChoosingPolicy implements VolumeChoosingPolicy {
public static final Logger LOG = LoggerFactory.getLogger(
CapacityVolumeChoosingPolicy.class);

// Stores the index of the next volume to be returned.
private final Random random = new Random();
Comment on lines -47 to -48
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea here was to share the Random instance among all threads. Is this change necessary, or just cleanup?


@Override
public HddsVolume chooseVolume(List<HddsVolume> volumes,
long maxContainerSize) throws IOException {
Expand Down Expand Up @@ -83,6 +80,7 @@ public HddsVolume chooseVolume(List<HddsVolume> volumes,
// 4. vol2 + vol2: 25%, result is vol2
// So we have a total of 75% chances to choose vol1, which meets our
// expectation.
ThreadLocalRandom random = ThreadLocalRandom.current();
int firstIndex = random.nextInt(count);
int secondIndex = random.nextInt(count);

Expand Down
22 changes: 22 additions & 0 deletions ...ain/java/org/apache/hadoop/ozone/container/common/volume/VolumeChoosingPolicyFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public final class VolumeChoosingPolicyFactory {
private static final Class<? extends VolumeChoosingPolicy>
DEFAULT_VOLUME_CHOOSING_POLICY = CapacityVolumeChoosingPolicy.class;

private static volatile VolumeChoosingPolicy policyInstance;

private VolumeChoosingPolicyFactory() {
}

Expand All @@ -41,4 +43,24 @@ public static VolumeChoosingPolicy getPolicy(ConfigurationSource conf)
DEFAULT_VOLUME_CHOOSING_POLICY, VolumeChoosingPolicy.class)
.newInstance();
}

/**
* Get a shared singleton instance of VolumeChoosingPolicy.
* This method ensures that only one instance of VolumeChoosingPolicy
* is created and shared across the application.
*
* @param conf Configuration
* @return The shared VolumeChoosingPolicy instance
*/
public static VolumeChoosingPolicy getSharedPolicy(ConfigurationSource conf)
throws InstantiationException, IllegalAccessException {
if (policyInstance == null) {
synchronized (VolumeChoosingPolicyFactory.class) {
if (policyInstance == null) {
policyInstance = getPolicy(conf);
}
}
}
return policyInstance;
Copy link
Contributor

Choose a reason for hiding this comment

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

If policyInstance is already set, conf is ignored. It may be requesting a different implementation, which could be an error.

}
}
2 changes: 1 addition & 1 deletion ...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public KeyValueHandler(ConfigurationSource config,
chunkManager = ChunkManagerFactory.createChunkManager(config, blockManager,
volSet);
try {
volumeChoosingPolicy = VolumeChoosingPolicyFactory.getPolicy(conf);
volumeChoosingPolicy = VolumeChoosingPolicyFactory.getSharedPolicy(conf);
} catch (Exception e) {
throw new RuntimeException(e);
}
Expand Down
2 changes: 1 addition & 1 deletion ...ervice/src/main/java/org/apache/hadoop/ozone/container/replication/ContainerImporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public ContainerImporter(@Nonnull ConfigurationSource conf,
this.controller = controller;
this.volumeSet = volumeSet;
try {
volumeChoosingPolicy = VolumeChoosingPolicyFactory.getPolicy(conf);
volumeChoosingPolicy = VolumeChoosingPolicyFactory.getSharedPolicy(conf);
} catch (Exception e) {
throw new RuntimeException(e);
}
Expand Down
26 changes: 15 additions & 11 deletions ...service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,21 @@ public void testVolumeSetInKeyValueHandler() throws Exception {
DatanodeDetails datanodeDetails = mock(DatanodeDetails.class);
StateContext context = ContainerTestUtils.getMockContext(
datanodeDetails, conf);

//Set a class which is not of sub class of VolumeChoosingPolicy
conf.set(HDDS_DATANODE_VOLUME_CHOOSING_POLICY,
"org.apache.hadoop.ozone.container.common.impl.HddsDispatcher");
RuntimeException exception = assertThrows(RuntimeException.class,
() -> new KeyValueHandler(conf, context.getParent().getDatanodeDetails().getUuidString(), cset, volumeSet,
metrics, c -> {
}));

assertThat(exception).hasMessageEndingWith(
"class org.apache.hadoop.ozone.container.common.impl.HddsDispatcher " +
"not org.apache.hadoop.ozone.container.common.interfaces.VolumeChoosingPolicy");

conf.set(HDDS_DATANODE_VOLUME_CHOOSING_POLICY,
"org.apache.hadoop.ozone.container.common.volume.CapacityVolumeChoosingPolicy");
KeyValueHandler keyValueHandler = new KeyValueHandler(conf,
context.getParent().getDatanodeDetails().getUuidString(), cset,
volumeSet, metrics, c -> {
Expand All @@ -297,17 +312,6 @@ public void testVolumeSetInKeyValueHandler() throws Exception {
metrics, c -> { });
assertEquals(ContainerLayoutVersion.FILE_PER_BLOCK,
conf.getEnum(OZONE_SCM_CONTAINER_LAYOUT_KEY, ContainerLayoutVersion.FILE_PER_CHUNK));

//Set a class which is not of sub class of VolumeChoosingPolicy
conf.set(HDDS_DATANODE_VOLUME_CHOOSING_POLICY,
"org.apache.hadoop.ozone.container.common.impl.HddsDispatcher");
RuntimeException exception = assertThrows(RuntimeException.class,
() -> new KeyValueHandler(conf, context.getParent().getDatanodeDetails().getUuidString(), cset, volumeSet,
metrics, c -> { }));

assertThat(exception).hasMessageEndingWith(
"class org.apache.hadoop.ozone.container.common.impl.HddsDispatcher " +
"not org.apache.hadoop.ozone.container.common.interfaces.VolumeChoosingPolicy");
Comment on lines -300 to -310
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this to the start of the test case is not enough, the test may still fail if other test cases are run first. This is a problem with singletons.

} finally {
volumeSet.shutdown();
FileUtil.fullyDelete(datanodeDir);
Expand Down