chungen0126 commented on code in PR #10573:
URL: https://github.com/apache/ozone/pull/10573#discussion_r3451850771
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -170,6 +178,32 @@ public DatanodeID getSuggestedLeaderId() {
return suggestedLeaderId;
}
+ /**
+ * Return the Pipeline supported StorageTier.
+ *
+ * @return Supported StorageTier
+ */
+ public List<StorageTier> getSupportedStorageTier() {
+ lock.readLock().lock();
+ try {
+ return supportedStorageTier;
+ } finally {
+ lock.readLock().unlock();
+ }
+ }
+
+ /**
+ * Set the storageTier supported by the pipeline.
+ */
+ public void setSupportedStorageTier(List<StorageTier> supportedStorageTier) {
Review Comment:
Using setSupportedStorageTier here breaks the immutable design of the
Pipeline class. A better approach would be to follow the existing pattern of
copyWithNodesInOrder to update the pipeline's state.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -722,6 +724,49 @@ public void processNodeReport(DatanodeDetails
datanodeDetails,
}
}
+ private void updateSupportedStorageTier(DatanodeInfo datanodeInfo) {
+ if (scmContext.getScm() == null) {
Review Comment:
Would it be better to use `instanceof`?
```suggestion
if (scmContext.getScm() instanceof StorageContainerManager) {
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -722,6 +724,49 @@ public void processNodeReport(DatanodeDetails
datanodeDetails,
}
}
+ private void updateSupportedStorageTier(DatanodeInfo datanodeInfo) {
+ if (scmContext.getScm() == null) {
+ LOG.debug("Skip the updating of Pipeline supported StorageTier for
Recon");
+ return;
+ }
+
+ PipelineManager pipelineManager = scmContext.getScm().getPipelineManager();
+ if (pipelineManager == null) {
+ LOG.debug("Skip the updating of Pipeline supported StorageTier for
Recon");
+ return;
+ }
+
+ Set<PipelineID> pipelines = nodeStateManager.getPipelineByDnID(
+ datanodeInfo.getID());
+ for (PipelineID pipelineId : pipelines) {
+ try {
+ Pipeline pipeline = pipelineManager.getPipeline(pipelineId);
+ Set<StorageTier> currentTiers =
+ pipeline.getSupportedStorageTier() != null
+ ? new HashSet<>(pipeline.getSupportedStorageTier())
+ : Collections.emptySet();
+ List<StorageTier> newSupportedTiers =
+ NodeUtils.getDatanodesStorageTypes(pipeline.getNodes(), this);
+ Set<StorageTier> newSupportedTierSet =
+ new HashSet<>(newSupportedTiers);
+ if (!currentTiers.equals(newSupportedTierSet)) {
+ pipeline.setSupportedStorageTier(newSupportedTiers);
+ LOG.info("Updated supported storage tiers for Pipeline ID {} from {}
"
+ + "to {} by Datanode {}",
+ pipeline.getId(), currentTiers, newSupportedTierSet,
+ datanodeInfo.getID());
+ }
+ } catch (PipelineNotFoundException e) {
+ LOG.debug("Reported Datanode {} pipeline {} is not found",
Review Comment:
Would it be better to use the warn log level here?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -722,6 +724,49 @@ public void processNodeReport(DatanodeDetails
datanodeDetails,
}
}
+ private void updateSupportedStorageTier(DatanodeInfo datanodeInfo) {
+ if (scmContext.getScm() == null) {
+ LOG.debug("Skip the updating of Pipeline supported StorageTier for
Recon");
+ return;
+ }
+
+ PipelineManager pipelineManager = scmContext.getScm().getPipelineManager();
+ if (pipelineManager == null) {
+ LOG.debug("Skip the updating of Pipeline supported StorageTier for
Recon");
+ return;
+ }
+
+ Set<PipelineID> pipelines = nodeStateManager.getPipelineByDnID(
+ datanodeInfo.getID());
+ for (PipelineID pipelineId : pipelines) {
+ try {
+ Pipeline pipeline = pipelineManager.getPipeline(pipelineId);
+ Set<StorageTier> currentTiers =
+ pipeline.getSupportedStorageTier() != null
+ ? new HashSet<>(pipeline.getSupportedStorageTier())
+ : Collections.emptySet();
+ List<StorageTier> newSupportedTiers =
+ NodeUtils.getDatanodesStorageTypes(pipeline.getNodes(), this);
+ Set<StorageTier> newSupportedTierSet =
+ new HashSet<>(newSupportedTiers);
+ if (!currentTiers.equals(newSupportedTierSet)) {
+ pipeline.setSupportedStorageTier(newSupportedTiers);
+ LOG.info("Updated supported storage tiers for Pipeline ID {} from {}
"
+ + "to {} by Datanode {}",
+ pipeline.getId(), currentTiers, newSupportedTierSet,
+ datanodeInfo.getID());
+ }
Review Comment:
I'm not sure if we should update the pipeline when processing node report.
My understanding is that pipeline updates should be scoped exclusively to
`PipelineReportHandler`
--
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]
