This is an automated email from the ASF dual-hosted git repository.
szetszwo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new de73c00a8d HDDS-12738. Refactor AbstractContainerReportHandler and its
subclasses. (#8207)
de73c00a8d is described below
commit de73c00a8d4526ec5ddf7609327ea9646ad84ffb
Author: Tsz-Wo Nicholas Sze <[email protected]>
AuthorDate: Wed Apr 2 13:54:00 2025 -0700
HDDS-12738. Refactor AbstractContainerReportHandler and its subclasses.
(#8207)
---
.../container/AbstractContainerReportHandler.java | 31 ++++------
.../hdds/scm/container/ContainerReportHandler.java | 68 +++++++++++-----------
.../IncrementalContainerReportHandler.java | 44 ++++++--------
.../scm/container/TestUnknownContainerReport.java | 2 +-
.../recon/scm/ReconContainerReportHandler.java | 8 +++
.../ReconIncrementalContainerReportHandler.java | 5 ++
6 files changed, 76 insertions(+), 82 deletions(-)
diff --git
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
index 17029a915e..e3c903da2d 100644
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
@@ -33,6 +33,7 @@
import
org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State;
import org.apache.hadoop.hdds.scm.events.SCMEvents;
import org.apache.hadoop.hdds.scm.ha.SCMContext;
+import org.apache.hadoop.hdds.scm.node.NodeManager;
import org.apache.hadoop.hdds.server.events.EventPublisher;
import
org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
import org.apache.hadoop.ozone.protocol.commands.CommandForDatanode;
@@ -46,30 +47,18 @@
/**
* Base class for all the container report handlers.
*/
-public class AbstractContainerReportHandler {
-
+abstract class AbstractContainerReportHandler {
+ private final NodeManager nodeManager;
private final ContainerManager containerManager;
private final SCMContext scmContext;
- private final Logger logger;
- /**
- * Constructs AbstractContainerReportHandler instance with the
- * given ContainerManager instance.
- *
- * @param containerManager ContainerManager
- * @param logger Logger to be used for logging
- */
- AbstractContainerReportHandler(final ContainerManager containerManager,
- final SCMContext scmContext,
- final Logger logger) {
+ AbstractContainerReportHandler(NodeManager nodeManager, ContainerManager
containerManager, SCMContext scmContext) {
+ this.nodeManager = Objects.requireNonNull(nodeManager, "nodeManager ==
null");
this.containerManager = Objects.requireNonNull(containerManager,
"containerManager == null");
this.scmContext = Objects.requireNonNull(scmContext, "scmContext == null");
- this.logger = Objects.requireNonNull(logger, "logger == null");
}
- protected Logger getLogger() {
- return logger;
- }
+ protected abstract Logger getLogger();
/** @return the container in SCM and the replica from a datanode details for
logging. */
static Object getDetailsForLogging(ContainerInfo container,
ContainerReplicaProto replica, DatanodeDetails datanode) {
@@ -414,10 +403,10 @@ private boolean isHealthy(final State replicaState) {
&& replicaState != State.DELETED;
}
- /**
- * Return ContainerManager.
- * @return {@link ContainerManager}
- */
+ protected NodeManager getNodeManager() {
+ return nodeManager;
+ }
+
protected ContainerManager getContainerManager() {
return containerManager;
}
diff --git
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
index 4e41f962e2..288c479ba2 100644
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
@@ -47,16 +47,15 @@ public class ContainerReportHandler extends
AbstractContainerReportHandler
private static final Logger LOG =
LoggerFactory.getLogger(ContainerReportHandler.class);
- private final NodeManager nodeManager;
- private final ContainerManager containerManager;
- private final String unknownContainerHandleAction;
+ enum UnknownContainerAction {
+ WARN, DELETE;
- /**
- * The action taken by ContainerReportHandler to handle
- * unknown containers.
- */
- static final String UNKNOWN_CONTAINER_ACTION_WARN = "WARN";
- static final String UNKNOWN_CONTAINER_ACTION_DELETE = "DELETE";
+ static UnknownContainerAction parse(String s) {
+ return s.equals(DELETE.name()) ? DELETE : WARN;
+ }
+ }
+
+ private final UnknownContainerAction unknownContainerHandleAction;
/**
* Constructs ContainerReportHandler instance with the
@@ -70,18 +69,21 @@ public ContainerReportHandler(final NodeManager nodeManager,
final ContainerManager containerManager,
final SCMContext scmContext,
OzoneConfiguration conf) {
- super(containerManager, scmContext, LOG);
- this.nodeManager = nodeManager;
- this.containerManager = containerManager;
+ super(nodeManager, containerManager, scmContext);
if (conf != null) {
ScmConfig scmConfig = conf.getObject(ScmConfig.class);
- unknownContainerHandleAction = scmConfig.getUnknownContainerAction();
+ unknownContainerHandleAction =
UnknownContainerAction.parse(scmConfig.getUnknownContainerAction());
} else {
- unknownContainerHandleAction = UNKNOWN_CONTAINER_ACTION_WARN;
+ unknownContainerHandleAction = UnknownContainerAction.WARN;
}
}
+ @Override
+ protected Logger getLogger() {
+ return LOG;
+ }
+
public ContainerReportHandler(final NodeManager nodeManager,
final ContainerManager containerManager) {
this(nodeManager, containerManager, SCMContext.emptyContext(), null);
@@ -142,10 +144,9 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
final DatanodeDetails dnFromReport =
reportFromDatanode.getDatanodeDetails();
- final DatanodeDetails datanodeDetails =
nodeManager.getNode(dnFromReport.getID());
+ final DatanodeDetails datanodeDetails =
getNodeManager().getNode(dnFromReport.getID());
if (datanodeDetails == null) {
- LOG.warn("Received container report from unknown datanode {}",
- dnFromReport);
+ getLogger().warn("Datanode not found: {}", dnFromReport);
return;
}
final ContainerReportsProto containerReport =
@@ -159,7 +160,7 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
final List<ContainerReplicaProto> replicas =
containerReport.getReportsList();
final Set<ContainerID> expectedContainersInDatanode =
- nodeManager.getContainers(datanodeDetails);
+ getNodeManager().getContainers(datanodeDetails);
for (ContainerReplicaProto replica : replicas) {
ContainerID cid = ContainerID.valueOf(replica.getContainerID());
@@ -169,7 +170,7 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
// from protobuf. However we don't want to store that object if
// there is already an instance for the same ContainerID we can
// reuse.
- container = containerManager.getContainer(cid);
+ container = getContainerManager().getContainer(cid);
cid = container.containerID();
} catch (ContainerNotFoundException e) {
// Ignore this for now. It will be handled later with a null check
@@ -181,7 +182,7 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
boolean alreadyInDn = expectedContainersInDatanode.remove(cid);
if (!alreadyInDn) {
// This is a new Container not in the nodeManager -> dn map yet
- nodeManager.addContainer(datanodeDetails, cid);
+ getNodeManager().addContainer(datanodeDetails, cid);
}
if (container == null || ContainerReportValidator
.validate(container, datanodeDetails, replica)) {
@@ -193,7 +194,7 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
// report, so it is now missing on the DN. We need to remove it from
the
// list
processMissingReplicas(datanodeDetails, expectedContainersInDatanode);
- containerManager.notifyContainerReportProcessing(true, true);
+ getContainerManager().notifyContainerReportProcessing(true, true);
if (reportFromDatanode.isRegister()) {
publisher.fireEvent(SCMEvents.CONTAINER_REGISTRATION_REPORT,
new
SCMDatanodeProtocolServer.NodeRegistrationContainerReport(datanodeDetails,
@@ -201,9 +202,8 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
}
}
} catch (NodeNotFoundException ex) {
- containerManager.notifyContainerReportProcessing(true, false);
- LOG.error("Received container report from unknown datanode {}.",
- datanodeDetails, ex);
+ getContainerManager().notifyContainerReportProcessing(true, false);
+ getLogger().warn("Datanode not found: {}", datanodeDetails, ex);
}
}
@@ -224,11 +224,9 @@ private void processSingleReplica(final DatanodeDetails
datanodeDetails,
final EventPublisher publisher) {
final Object detailsForLogging = getDetailsForLogging(container,
replicaProto, datanodeDetails);
if (container == null) {
- if (unknownContainerHandleAction.equals(
- UNKNOWN_CONTAINER_ACTION_WARN)) {
+ if (unknownContainerHandleAction == UnknownContainerAction.WARN) {
getLogger().error("CONTAINER_NOT_FOUND for {}", detailsForLogging);
- } else if (unknownContainerHandleAction.equals(
- UNKNOWN_CONTAINER_ACTION_DELETE)) {
+ } else if (unknownContainerHandleAction ==
UnknownContainerAction.DELETE) {
final ContainerID containerId = ContainerID
.valueOf(replicaProto.getContainerID());
deleteReplica(containerId, datanodeDetails, publisher,
"CONTAINER_NOT_FOUND", true, detailsForLogging);
@@ -253,26 +251,26 @@ private void processMissingReplicas(final DatanodeDetails
datanodeDetails,
final Set<ContainerID> missingReplicas) {
for (ContainerID id : missingReplicas) {
try {
- nodeManager.removeContainer(datanodeDetails, id);
+ getNodeManager().removeContainer(datanodeDetails, id);
} catch (NodeNotFoundException e) {
- LOG.warn("Failed to remove container {} from a node which does not " +
- "exist {}", id, datanodeDetails, e);
+ getLogger().warn("Failed to remove missing container {}: datanode {}
not found",
+ id, datanodeDetails, e);
}
try {
- containerManager.getContainerReplicas(id).stream()
+ getContainerManager().getContainerReplicas(id).stream()
.filter(replica -> replica.getDatanodeDetails()
.equals(datanodeDetails)).findFirst()
.ifPresent(replica -> {
try {
- containerManager.removeContainerReplica(id, replica);
+ getContainerManager().removeContainerReplica(id, replica);
} catch (ContainerNotFoundException |
ContainerReplicaNotFoundException ignored) {
// This should not happen, but even if it happens, not an issue
}
});
} catch (ContainerNotFoundException e) {
- LOG.warn("Cannot remove container replica, container {} not found.",
- id, e);
+ getLogger().warn("Failed to remove container replica: container {} not
found in datanode {}",
+ id, datanodeDetails, e);
}
}
}
diff --git
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/IncrementalContainerReportHandler.java
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/IncrementalContainerReportHandler.java
index a1bd9010f8..72722153b9 100644
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/IncrementalContainerReportHandler.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/IncrementalContainerReportHandler.java
@@ -20,6 +20,7 @@
import java.io.IOException;
import org.apache.hadoop.hdds.protocol.DatanodeDetails;
import
org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import
org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State;
import org.apache.hadoop.hdds.scm.container.report.ContainerReportValidator;
import org.apache.hadoop.hdds.scm.exceptions.SCMException;
import org.apache.hadoop.hdds.scm.ha.SCMContext;
@@ -42,27 +43,26 @@ public class IncrementalContainerReportHandler extends
private static final Logger LOG = LoggerFactory.getLogger(
IncrementalContainerReportHandler.class);
- private final NodeManager nodeManager;
-
public IncrementalContainerReportHandler(
final NodeManager nodeManager,
final ContainerManager containerManager,
final SCMContext scmContext) {
- super(containerManager, scmContext, LOG);
- this.nodeManager = nodeManager;
+ super(nodeManager, containerManager, scmContext);
+ }
+
+ @Override
+ protected Logger getLogger() {
+ return LOG;
}
@Override
public void onMessage(final IncrementalContainerReportFromDatanode report,
final EventPublisher publisher) {
final DatanodeDetails dnFromReport = report.getDatanodeDetails();
- if (LOG.isDebugEnabled()) {
- LOG.debug("Processing incremental container report from data node {}",
dnFromReport);
- }
- final DatanodeDetails dd = nodeManager.getNode(dnFromReport.getID());
+ getLogger().debug("Processing incremental container report from datanode
{}", dnFromReport);
+ final DatanodeDetails dd = getNodeManager().getNode(dnFromReport.getID());
if (dd == null) {
- LOG.warn("Received container report from unknown datanode {}",
- dnFromReport);
+ getLogger().warn("Datanode not found: {}", dnFromReport);
return;
}
@@ -82,11 +82,10 @@ public void onMessage(final
IncrementalContainerReportFromDatanode report,
// Ensure we reuse the same ContainerID instance in containerInfo
id = container.containerID();
} finally {
- if (replicaProto.getState().equals(
- ContainerReplicaProto.State.DELETED)) {
- nodeManager.removeContainer(dd, id);
+ if (replicaProto.getState() == State.DELETED) {
+ getNodeManager().removeContainer(dd, id);
} else {
- nodeManager.addContainer(dd, id);
+ getNodeManager().addContainer(dd, id);
}
}
if (ContainerReportValidator.validate(container, dd, replicaProto)) {
@@ -94,23 +93,22 @@ public void onMessage(final
IncrementalContainerReportFromDatanode report,
}
success = true;
} catch (ContainerNotFoundException e) {
- LOG.warn("Container {} not found!", replicaProto.getContainerID());
+ getLogger().warn("Container {} not found!",
replicaProto.getContainerID());
} catch (NodeNotFoundException ex) {
- LOG.error("Received ICR from unknown datanode {}",
- report.getDatanodeDetails(), ex);
+ getLogger().error("Datanode not found {}",
report.getDatanodeDetails(), ex);
} catch (ContainerReplicaNotFoundException e) {
- LOG.warn("Container {} replica not found!",
+ getLogger().warn("Container {} replica not found!",
replicaProto.getContainerID());
} catch (SCMException ex) {
if (ex.getResult() == SCMException.ResultCodes.SCM_NOT_LEADER) {
- LOG.info("Failed to process {} container {}: {}",
+ getLogger().info("Failed to process {} container {}: {}",
replicaProto.getState(), id, ex.getMessage());
} else {
- LOG.error("Exception while processing ICR for container {}",
+ getLogger().error("Exception while processing ICR for container
{}",
replicaProto.getContainerID(), ex);
}
} catch (IOException | InvalidStateTransitionException e) {
- LOG.error("Exception while processing ICR for container {}",
+ getLogger().error("Exception while processing ICR for container {}",
replicaProto.getContainerID(), e);
}
}
@@ -118,8 +116,4 @@ public void onMessage(final
IncrementalContainerReportFromDatanode report,
getContainerManager().notifyContainerReportProcessing(false, success);
}
-
- protected NodeManager getNodeManager() {
- return this.nodeManager;
- }
}
diff --git
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestUnknownContainerReport.java
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestUnknownContainerReport.java
index bfb08322a8..369d1142ea 100644
---
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestUnknownContainerReport.java
+++
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestUnknownContainerReport.java
@@ -115,7 +115,7 @@ public void testUnknownContainerDeleted() {
OzoneConfiguration conf = new OzoneConfiguration();
conf.set(
ScmConfig.HDDS_SCM_UNKNOWN_CONTAINER_ACTION,
- ContainerReportHandler.UNKNOWN_CONTAINER_ACTION_DELETE);
+ ContainerReportHandler.UnknownContainerAction.DELETE.name());
sendContainerReport(conf);
verify(publisher, times(1)).fireEvent(
diff --git
a/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerReportHandler.java
b/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerReportHandler.java
index adbe00b8fb..6b0f5f86b0 100644
---
a/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerReportHandler.java
+++
b/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerReportHandler.java
@@ -24,17 +24,25 @@
import org.apache.hadoop.hdds.scm.node.NodeManager;
import
org.apache.hadoop.hdds.scm.server.SCMDatanodeHeartbeatDispatcher.ContainerReportFromDatanode;
import org.apache.hadoop.hdds.server.events.EventPublisher;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Recon's container report handler.
*/
public class ReconContainerReportHandler extends ContainerReportHandler {
+ private static final Logger LOG =
LoggerFactory.getLogger(ReconContainerReportHandler.class);
public ReconContainerReportHandler(NodeManager nodeManager,
ContainerManager containerManager) {
super(nodeManager, containerManager);
}
+ @Override
+ protected Logger getLogger() {
+ return LOG;
+ }
+
@Override
public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final EventPublisher publisher) {
diff --git
a/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconIncrementalContainerReportHandler.java
b/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconIncrementalContainerReportHandler.java
index 3d3160c80a..c6f10c9b5b 100644
---
a/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconIncrementalContainerReportHandler.java
+++
b/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconIncrementalContainerReportHandler.java
@@ -47,6 +47,11 @@ public ReconIncrementalContainerReportHandler(NodeManager
nodeManager,
super(nodeManager, containerManager, scmContext);
}
+ @Override
+ protected Logger getLogger() {
+ return LOG;
+ }
+
@Override
public void onMessage(final IncrementalContainerReportFromDatanode report,
final EventPublisher publisher) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
