Copilot commented on code in PR #11182:
URL: https://github.com/apache/ozone/pull/11182#discussion_r3905059222
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/reconciliation/ReconcileContainerEventHandler.java:
##########
@@ -67,21 +73,46 @@ public void onMessage(ContainerID containerID,
EventPublisher publisher) {
}
try {
- // TODO HDDS-10714 restriction peer and target nodes based on node
status.
- Set<DatanodeDetails> allReplicaNodes =
containerManager.getContainerReplicas(containerID)
- .stream()
- .map(ContainerReplica::getDatanodeDetails)
- .collect(Collectors.toSet());
+ Set<DatanodeDetails> targets = new HashSet<>();
+ Set<DatanodeDetails> peers = new HashSet<>();
+ for (ContainerReplica replica :
containerManager.getContainerReplicas(containerID)) {
+ DatanodeDetails datanode = replica.getDatanodeDetails();
+ final NodeStatus status;
+ try {
+ status = nodeManager.getNodeStatus(datanode);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("Skipping datanode {} for reconciliation of container {}
since its status is unknown.",
+ datanode, containerID);
+ continue;
+ }
+ if (!status.isHealthy()) {
+ continue;
+ }
+ // Transitioning nodes remain peers so other replicas can recover any
unique data before the node leaves.
+ if (!status.isDecommissioned() && !status.isInMaintenance()) {
+ peers.add(datanode);
+ }
+ if (status.isInService()) {
+ targets.add(datanode);
+ }
+ }
+
+ LOG.info("Reconcile container event triggered for container {} with
targets {} and peers {}",
+ containerID, targets, peers);
- LOG.info("Reconcile container event triggered for container {} with
peers {}", containerID, allReplicaNodes);
+ if (targets.isEmpty()) {
+ LOG.warn("Skipping reconciliation for container {} since no eligible
target datanodes are available.",
+ containerID);
+ return;
+ }
Review Comment:
Logging `targets` and `peers` at INFO will print full `DatanodeDetails`
sets, which can be very noisy (and potentially large) in busy clusters and
makes logs harder to operate. Prefer logging counts at INFO (eg. target/peer
sizes) and, if needed, log the full sets at DEBUG/TRACE. Also consider logging
after the empty-target early return so skipped reconciliations don’t emit a
large INFO line.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/reconciliation/ReconcileContainerEventHandler.java:
##########
@@ -67,21 +73,46 @@ public void onMessage(ContainerID containerID,
EventPublisher publisher) {
}
try {
- // TODO HDDS-10714 restriction peer and target nodes based on node
status.
- Set<DatanodeDetails> allReplicaNodes =
containerManager.getContainerReplicas(containerID)
- .stream()
- .map(ContainerReplica::getDatanodeDetails)
- .collect(Collectors.toSet());
+ Set<DatanodeDetails> targets = new HashSet<>();
+ Set<DatanodeDetails> peers = new HashSet<>();
+ for (ContainerReplica replica :
containerManager.getContainerReplicas(containerID)) {
+ DatanodeDetails datanode = replica.getDatanodeDetails();
+ final NodeStatus status;
+ try {
+ status = nodeManager.getNodeStatus(datanode);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("Skipping datanode {} for reconciliation of container {}
since its status is unknown.",
+ datanode, containerID);
+ continue;
+ }
+ if (!status.isHealthy()) {
+ continue;
+ }
+ // Transitioning nodes remain peers so other replicas can recover any
unique data before the node leaves.
+ if (!status.isDecommissioned() && !status.isInMaintenance()) {
+ peers.add(datanode);
+ }
+ if (status.isInService()) {
+ targets.add(datanode);
+ }
+ }
Review Comment:
This loops over replicas and calls `nodeManager.getNodeStatus(datanode)` per
replica. If a container can have multiple replicas reported for the same
datanode (or if `getContainerReplicas` is large), this causes redundant status
lookups and repeated warn logs for the same node. Consider deduplicating
`DatanodeDetails` first (or caching statuses in a `Map<DatanodeDetails,
NodeStatus>` within the method) so each datanode’s status is fetched at most
once per reconciliation event.
--
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]
