Copilot commented on code in PR #10865:
URL: https://github.com/apache/ozone/pull/10865#discussion_r3660524782
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java:
##########
@@ -224,14 +231,19 @@ public Pipeline createForRead(
.build();
}
- private List<DatanodeDetails> filterPipelineEngagement() {
+ /**
+ *
+ * @return
+ */
+ private List<DatanodeDetails> filterNodes(boolean filterRatisStreaming) {
Review Comment:
This Javadoc is empty/placeholder (blank description and `@return` without
details), which is likely to violate common Javadoc/checkstyle rules and is not
helpful for future maintenance. Either remove the Javadoc entirely or document
what is being filtered/excluded and what the returned list represents (eg,
'nodes to exclude due to pipeline limit and optionally missing
RATIS_DATASTREAM').
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java:
##########
@@ -155,25 +157,30 @@ public synchronized Pipeline
create(RatisReplicationConfig replicationConfig,
);
}
- final List<DatanodeDetails> dns;
+ List<DatanodeDetails> dns;
final ReplicationFactor factor =
replicationConfig.getReplicationFactor();
switch (factor) {
case ONE:
dns = pickNodesNotUsed(replicationConfig, minRatisVolumeSizeBytes,
containerSizeBytes);
break;
case THREE:
- List<DatanodeDetails> excludeDueToEngagement =
filterPipelineEngagement();
- if (!excludeDueToEngagement.isEmpty()) {
- if (excludedNodes.isEmpty()) {
- excludedNodes = excludeDueToEngagement;
- } else {
- excludedNodes.addAll(excludeDueToEngagement);
- }
+ List<DatanodeDetails> excludeDueToEngagement = filterNodes(true);
Review Comment:
The variable name excludeDueToEngagement is misleading after the refactor:
filterNodes(true) now excludes both over-utilized nodes and nodes missing
RATIS_DATASTREAM capability. Renaming it to something like excludedCandidates /
excludedNodesThisAttempt / excludedByFilters would make the intent clearer and
reduce confusion for future readers.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java:
##########
@@ -155,25 +157,30 @@ public synchronized Pipeline
create(RatisReplicationConfig replicationConfig,
);
}
- final List<DatanodeDetails> dns;
+ List<DatanodeDetails> dns;
final ReplicationFactor factor =
replicationConfig.getReplicationFactor();
switch (factor) {
case ONE:
dns = pickNodesNotUsed(replicationConfig, minRatisVolumeSizeBytes,
containerSizeBytes);
break;
case THREE:
- List<DatanodeDetails> excludeDueToEngagement =
filterPipelineEngagement();
- if (!excludeDueToEngagement.isEmpty()) {
- if (excludedNodes.isEmpty()) {
- excludedNodes = excludeDueToEngagement;
- } else {
- excludedNodes.addAll(excludeDueToEngagement);
- }
+ List<DatanodeDetails> excludeDueToEngagement = filterNodes(true);
+ List<DatanodeDetails> currentExcluded = new ArrayList<>(excludedNodes);
+ currentExcluded.addAll(excludeDueToEngagement);
+ try {
+ dns = placementPolicy.chooseDatanodes(currentExcluded,
+ favoredNodes, factor.getNumber(), minRatisVolumeSizeBytes,
+ containerSizeBytes);
+ } catch (SCMException scmException) {
+ excludeDueToEngagement = filterNodes(false);
+ currentExcluded = new ArrayList<>(excludedNodes);
+ currentExcluded.addAll(excludeDueToEngagement);
+ dns = placementPolicy.chooseDatanodes(currentExcluded,
+ favoredNodes, factor.getNumber(), minRatisVolumeSizeBytes,
+ containerSizeBytes);
Review Comment:
The fallback is triggered for any SCMException from chooseDatanodes, which
can unintentionally mask real placement/configuration errors (eg, invalid
parameters, topology errors, or other non-capability-related failures) by
retrying with relaxed filtering. Restrict the fallback to only the specific
failure indicating insufficient nodes after capability filtering (eg, check
scmException result code / cause) and rethrow for other SCMException types.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestRatisPipelineProvider.java:
##########
@@ -246,6 +261,73 @@ public void
testCreateFactorTHREEPipelineWithSameDatanodes()
assertEquals(pipeline2.getNodeSet(), pipeline3.getNodeSet());
}
+ private DatanodeDetails createDatanodeDetails(boolean supportRatisStreaming)
{
+ Random random = ThreadLocalRandom.current();
+ String ipAddress = random.nextInt(256)
+ + "." + random.nextInt(256)
+ + "." + random.nextInt(256)
+ + "." + random.nextInt(256);
Review Comment:
Using randomness to generate IP/hostnames in unit tests can make failures
harder to reproduce and can (rarely) introduce collisions. Prefer deterministic
addresses (eg, based on a monotonically increasing counter passed into the
helper) so the test inputs are stable and debuggable.
--
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]
