adoroszlai commented on code in PR #9464:
URL: https://github.com/apache/ozone/pull/9464#discussion_r2619545603
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/OneReplicaPipelineSafeModeRule.java:
##########
@@ -85,11 +86,19 @@ protected TypedEvent<PipelineReportFromDatanode>
getEventType() {
@Override
protected synchronized boolean validate() {
+ if (validateBasedOnReportProcessing()) {
+ return currentReportedPipelineCount >= thresholdCount;
+ }
+
+ updateReportedPipelineSet();
return currentReportedPipelineCount >= thresholdCount;
Review Comment:
nit:
```suggestion
if (!validateBasedOnReportProcessing()) {
updateReportedPipelineSet();
}
return currentReportedPipelineCount >= thresholdCount;
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/OneReplicaPipelineSafeModeRule.java:
##########
@@ -137,6 +146,10 @@ public synchronized int getCurrentReportedPipelineCount() {
return currentReportedPipelineCount;
}
+ public Set<PipelineID> getReportedPipelineIDSet() {
+ return reportedPipelineIDSet;
Review Comment:
Avoid exposing modifiable state. Also, it does not need to be `public`.
(needs import)
```suggestion
Set<PipelineID> getReportedPipelineIDSet() {
return Collections.unmodifiableSet(reportedPipelineIDSet);
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/OneReplicaPipelineSafeModeRule.java:
##########
@@ -171,6 +184,25 @@ public synchronized void refresh(boolean forceRefresh) {
}
}
+ private void updateReportedPipelineSet() {
+ List<Pipeline> openRatisPipelines =
+
pipelineManager.getPipelines(RatisReplicationConfig.getInstance(ReplicationFactor.THREE),
+ Pipeline.PipelineState.OPEN);
+
+ for (Pipeline pipeline : openRatisPipelines) {
+ PipelineID pipelineID = pipeline.getId();
+ boolean atleastOnePipelineReported = !pipeline.getNodeSet().isEmpty();
+ boolean notAlreadyReported = !reportedPipelineIDSet.contains(pipelineID);
+ boolean wasExistingPipeline = oldPipelineIDSet.contains(pipelineID);
+
+ if (atleastOnePipelineReported && notAlreadyReported &&
wasExistingPipeline) {
+
getSafeModeMetrics().incCurrentHealthyPipelinesWithAtleastOneReplicaReportedCount();
+ currentReportedPipelineCount++;
+ reportedPipelineIDSet.add(pipelineID);
Review Comment:
- Please inline the variables to avoid eagerly checking all 3 conditions.
- nit: we can skip `!reportedPipelineIDSet.contains(pipelineID)` by adding
`reportedPipelineIDSet.add(pipelineID)` as last condition
```suggestion
if (!pipeline.getNodeSet().isEmpty()
&& oldPipelineIDSet.contains(pipelineID)
&& reportedPipelineIDSet.add(pipelineID)) {
getSafeModeMetrics().incCurrentHealthyPipelinesWithAtleastOneReplicaReportedCount();
currentReportedPipelineCount++;
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/OneReplicaPipelineSafeModeRule.java:
##########
@@ -51,7 +52,7 @@ public class OneReplicaPipelineSafeModeRule extends
LoggerFactory.getLogger(OneReplicaPipelineSafeModeRule.class);
private int thresholdCount;
- private final Set<PipelineID> reportedPipelineIDSet = new HashSet<>();
+ private Set<PipelineID> reportedPipelineIDSet = new HashSet<>();
Review Comment:
Removing `final` doesn't seem to be necessary.
--
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]
