adoroszlai commented on code in PR #8090:
URL: https://github.com/apache/ozone/pull/8090#discussion_r2000192015
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeChoosingPolicyFactory.java:
##########
@@ -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;
Review Comment:
If `policyInstance` is already set, `conf` is ignored. It may be requesting
a different implementation, which could be an error.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/CapacityVolumeChoosingPolicy.java:
##########
@@ -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();
Review Comment:
I think the idea here was to share the `Random` instance among all threads.
Is this change necessary, or just cleanup?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -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");
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
