szetszwo commented on code in PR #10842:
URL: https://github.com/apache/ozone/pull/10842#discussion_r3685176931
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -384,9 +385,27 @@ public synchronized boolean hasPort(Port.Name name) {
return false;
}
+ /**
+ * Whether this datanode's exposed ports differ from {@code other}'s.
+ * Compared as a set of name=value entries, since {@link Port#equals}
+ * ignores the port value.
+ *
+ * @param other another snapshot of this datanode
+ * @return true if the two port sets are not identical
+ */
+ public boolean portsChanged(DatanodeDetails other) {
+ return !portValues(this).equals(portValues(other));
+ }
+
+ private static Set<String> portValues(DatanodeDetails datanodeDetails) {
+ return datanodeDetails.getPorts().stream()
+ .map(port -> port.getName() + "=" + port.getValue())
+ .collect(Collectors.toSet());
Review Comment:
This method is inefficient. Below are not needed:
- getPorts() copies the ports list.
- Create a Set of Strings.
However, the ports list itself is inefficient. Let's improve it in
HDDS-16043.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -182,13 +183,9 @@ public static PipelineManagerImpl newPipelineManager(
.setServiceName("BackgroundPipelineScrubber")
.setIntervalInMillis(scrubberIntervalInMillis)
.setWaitTimeInMillis(safeModeWaitMs)
- .setPeriodicalTask(() -> {
- try {
- pipelineManager.scrubPipelines();
- } catch (IOException e) {
- LOG.error("Unexpected error during pipeline scrubbing", e);
- }
- }).build();
+ .setPeriodicalTask(
+ pipelineManager::scrubAndClosePipelinesExposingNewPorts)
Review Comment:
Use a single line.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -562,6 +559,66 @@ static boolean
sameIdDifferentHostOrAddress(DatanodeDetails left, DatanodeDetail
|| !left.getHostName().equals(right.getHostName()));
}
+ /**
+ * Scrub pipelines, then close (and delete) OPEN RATIS pipelines whose
+ * registered nodes now advertise the RATIS_DATASTREAM port their stored node
+ * snapshot lacks.
+ */
+ public void scrubAndClosePipelinesExposingNewPorts() {
+ try {
+ scrubPipelines();
+ } catch (IOException e) {
+ LOG.error("Unexpected error during pipeline scrubbing", e);
+ }
+ closePipelinesExposingNewPorts();
+ }
+
+ void closePipelinesExposingNewPorts() {
Review Comment:
The code is checking only RATIS_DATASTREAM port. It is better call it
closePipelinesMissingDataStreamPort, which is consistent with
nodesMissingDataStreamPort.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -562,6 +559,66 @@ static boolean
sameIdDifferentHostOrAddress(DatanodeDetails left, DatanodeDetail
|| !left.getHostName().equals(right.getHostName()));
}
+ /**
+ * Scrub pipelines, then close (and delete) OPEN RATIS pipelines whose
+ * registered nodes now advertise the RATIS_DATASTREAM port their stored node
+ * snapshot lacks.
+ */
+ public void scrubAndClosePipelinesExposingNewPorts() {
+ try {
+ scrubPipelines();
+ } catch (IOException e) {
+ LOG.error("Unexpected error during pipeline scrubbing", e);
+ }
+ closePipelinesExposingNewPorts();
+ }
+
+ void closePipelinesExposingNewPorts() {
+ if (!isDataStreamEnabled()) {
+ return;
+ }
+ for (Pipeline pipeline : getPipelines()) {
+ if (!pipeline.isOpen()
+ || pipeline.getType() != ReplicationType.RATIS
+ || !nodesMissingDataStreamPort(pipeline)) {
+ continue;
+ }
+ try {
+ final PipelineID id = pipeline.getId();
+ LOG.info("Closing RATIS pipeline {} whose nodes now advertise the "
+ + "datastream port so a datastream-capable pipeline can replace
it",
+ id);
Review Comment:
Log messages should be shorter, simpler, and concise; see below.
<img width="562" height="369" alt="Image"
src="https://github.com/user-attachments/assets/8d53c239-1492-47ba-a929-e1fa2a1cadb1";
/>
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -474,6 +474,11 @@ public RegisteredCommand register(
"oldVersion = {}, newVersion = {}.",
datanodeDetails, oldNode.getVersion(),
datanodeDetails.getVersion());
nodeStateManager.updateNode(datanodeDetails, layoutInfo);
+ } else if (oldNode.portsChanged(datanodeDetails)) {
+ // Refresh the stored node when its port set changes
+ LOG.info("Updating ports for registered datanode {}: {} -> {}",
+ datanodeDetails, oldNode.getPorts(), datanodeDetails.getPorts());
+ nodeStateManager.updateNode(datanodeDetails, layoutInfo);
Review Comment:
@chihsuan , the port changed case should be handled similar to the version
changed case. It seems good to refresh everything.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -562,6 +559,66 @@ static boolean
sameIdDifferentHostOrAddress(DatanodeDetails left, DatanodeDetail
|| !left.getHostName().equals(right.getHostName()));
}
+ /**
+ * Scrub pipelines, then close (and delete) OPEN RATIS pipelines whose
+ * registered nodes now advertise the RATIS_DATASTREAM port their stored node
+ * snapshot lacks.
+ */
+ public void scrubAndClosePipelinesExposingNewPorts() {
+ try {
+ scrubPipelines();
+ } catch (IOException e) {
+ LOG.error("Unexpected error during pipeline scrubbing", e);
+ }
+ closePipelinesExposingNewPorts();
+ }
+
+ void closePipelinesExposingNewPorts() {
+ if (!isDataStreamEnabled()) {
+ return;
+ }
+ for (Pipeline pipeline : getPipelines()) {
Review Comment:
This is a one time thing happened when changing DataStream from disabled to
enabled. For simplicity, let's just close all the existing pipelines and
recreate new pipelines.
In practice, the maintenance window will be longer. So, it seems fine.
--
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]
