Skip to content

Commit de73c00

Browse files
szetszwoszetszwo
authored
HDDS-12738. Refactor AbstractContainerReportHandler and its subclasses. (apache#8207)
1 parent 30e4aa4 commit de73c00

File tree

6 files changed

+76
-82
lines changed

6 files changed

+76
-82
lines changed

‎hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State;
3434
import org.apache.hadoop.hdds.scm.events.SCMEvents;
3535
import org.apache.hadoop.hdds.scm.ha.SCMContext;
36+
import org.apache.hadoop.hdds.scm.node.NodeManager;
3637
import org.apache.hadoop.hdds.server.events.EventPublisher;
3738
import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
3839
import org.apache.hadoop.ozone.protocol.commands.CommandForDatanode;
@@ -46,30 +47,18 @@
4647
/**
4748
* Base class for all the container report handlers.
4849
*/
49-
public class AbstractContainerReportHandler {
50-
50+
abstract class AbstractContainerReportHandler {
51+
private final NodeManager nodeManager;
5152
private final ContainerManager containerManager;
5253
private final SCMContext scmContext;
53-
private final Logger logger;
5454

55-
/**
56-
* Constructs AbstractContainerReportHandler instance with the
57-
* given ContainerManager instance.
58-
*
59-
* @param containerManager ContainerManager
60-
* @param logger Logger to be used for logging
61-
*/
62-
AbstractContainerReportHandler(final ContainerManager containerManager,
63-
final SCMContext scmContext,
64-
final Logger logger) {
55+
AbstractContainerReportHandler(NodeManager nodeManager, ContainerManager containerManager, SCMContext scmContext) {
56+
this.nodeManager = Objects.requireNonNull(nodeManager, "nodeManager == null");
6557
this.containerManager = Objects.requireNonNull(containerManager, "containerManager == null");
6658
this.scmContext = Objects.requireNonNull(scmContext, "scmContext == null");
67-
this.logger = Objects.requireNonNull(logger, "logger == null");
6859
}
6960

70-
protected Logger getLogger() {
71-
return logger;
72-
}
61+
protected abstract Logger getLogger();
7362

7463
/** @return the container in SCM and the replica from a datanode details for logging. */
7564
static Object getDetailsForLogging(ContainerInfo container, ContainerReplicaProto replica, DatanodeDetails datanode) {
@@ -414,10 +403,10 @@ private boolean isHealthy(final State replicaState) {
414403
&& replicaState != State.DELETED;
415404
}
416405

417-
/**
418-
* Return ContainerManager.
419-
* @return {@link ContainerManager}
420-
*/
406+
protected NodeManager getNodeManager() {
407+
return nodeManager;
408+
}
409+
421410
protected ContainerManager getContainerManager() {
422411
return containerManager;
423412
}

‎hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java

Lines changed: 33 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -47,16 +47,15 @@ public class ContainerReportHandler extends AbstractContainerReportHandler
4747
private static final Logger LOG =
4848
LoggerFactory.getLogger(ContainerReportHandler.class);
4949

50-
private final NodeManager nodeManager;
51-
private final ContainerManager containerManager;
52-
private final String unknownContainerHandleAction;
50+
enum UnknownContainerAction {
51+
WARN, DELETE;
5352

54-
/**
55-
* The action taken by ContainerReportHandler to handle
56-
* unknown containers.
57-
*/
58-
static final String UNKNOWN_CONTAINER_ACTION_WARN = "WARN";
59-
static final String UNKNOWN_CONTAINER_ACTION_DELETE = "DELETE";
53+
static UnknownContainerAction parse(String s) {
54+
return s.equals(DELETE.name()) ? DELETE : WARN;
55+
}
56+
}
57+
58+
private final UnknownContainerAction unknownContainerHandleAction;
6059

6160
/**
6261
* Constructs ContainerReportHandler instance with the
@@ -70,18 +69,21 @@ public ContainerReportHandler(final NodeManager nodeManager,
7069
final ContainerManager containerManager,
7170
final SCMContext scmContext,
7271
OzoneConfiguration conf) {
73-
super(containerManager, scmContext, LOG);
74-
this.nodeManager = nodeManager;
75-
this.containerManager = containerManager;
72+
super(nodeManager, containerManager, scmContext);
7673

7774
if (conf != null) {
7875
ScmConfig scmConfig = conf.getObject(ScmConfig.class);
79-
unknownContainerHandleAction = scmConfig.getUnknownContainerAction();
76+
unknownContainerHandleAction = UnknownContainerAction.parse(scmConfig.getUnknownContainerAction());
8077
} else {
81-
unknownContainerHandleAction = UNKNOWN_CONTAINER_ACTION_WARN;
78+
unknownContainerHandleAction = UnknownContainerAction.WARN;
8279
}
8380
}
8481

82+
@Override
83+
protected Logger getLogger() {
84+
return LOG;
85+
}
86+
8587
public ContainerReportHandler(final NodeManager nodeManager,
8688
final ContainerManager containerManager) {
8789
this(nodeManager, containerManager, SCMContext.emptyContext(), null);
@@ -142,10 +144,9 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
142144

143145
final DatanodeDetails dnFromReport =
144146
reportFromDatanode.getDatanodeDetails();
145-
final DatanodeDetails datanodeDetails = nodeManager.getNode(dnFromReport.getID());
147+
final DatanodeDetails datanodeDetails = getNodeManager().getNode(dnFromReport.getID());
146148
if (datanodeDetails == null) {
147-
LOG.warn("Received container report from unknown datanode {}",
148-
dnFromReport);
149+
getLogger().warn("Datanode not found: {}", dnFromReport);
149150
return;
150151
}
151152
final ContainerReportsProto containerReport =
@@ -159,7 +160,7 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
159160
final List<ContainerReplicaProto> replicas =
160161
containerReport.getReportsList();
161162
final Set<ContainerID> expectedContainersInDatanode =
162-
nodeManager.getContainers(datanodeDetails);
163+
getNodeManager().getContainers(datanodeDetails);
163164

164165
for (ContainerReplicaProto replica : replicas) {
165166
ContainerID cid = ContainerID.valueOf(replica.getContainerID());
@@ -169,7 +170,7 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
169170
// from protobuf. However we don't want to store that object if
170171
// there is already an instance for the same ContainerID we can
171172
// reuse.
172-
container = containerManager.getContainer(cid);
173+
container = getContainerManager().getContainer(cid);
173174
cid = container.containerID();
174175
} catch (ContainerNotFoundException e) {
175176
// Ignore this for now. It will be handled later with a null check
@@ -181,7 +182,7 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
181182
boolean alreadyInDn = expectedContainersInDatanode.remove(cid);
182183
if (!alreadyInDn) {
183184
// This is a new Container not in the nodeManager -> dn map yet
184-
nodeManager.addContainer(datanodeDetails, cid);
185+
getNodeManager().addContainer(datanodeDetails, cid);
185186
}
186187
if (container == null || ContainerReportValidator
187188
.validate(container, datanodeDetails, replica)) {
@@ -193,17 +194,16 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
193194
// report, so it is now missing on the DN. We need to remove it from the
194195
// list
195196
processMissingReplicas(datanodeDetails, expectedContainersInDatanode);
196-
containerManager.notifyContainerReportProcessing(true, true);
197+
getContainerManager().notifyContainerReportProcessing(true, true);
197198
if (reportFromDatanode.isRegister()) {
198199
publisher.fireEvent(SCMEvents.CONTAINER_REGISTRATION_REPORT,
199200
new SCMDatanodeProtocolServer.NodeRegistrationContainerReport(datanodeDetails,
200201
reportFromDatanode.getReport()));
201202
}
202203
}
203204
} catch (NodeNotFoundException ex) {
204-
containerManager.notifyContainerReportProcessing(true, false);
205-
LOG.error("Received container report from unknown datanode {}.",
206-
datanodeDetails, ex);
205+
getContainerManager().notifyContainerReportProcessing(true, false);
206+
getLogger().warn("Datanode not found: {}", datanodeDetails, ex);
207207
}
208208

209209
}
@@ -224,11 +224,9 @@ private void processSingleReplica(final DatanodeDetails datanodeDetails,
224224
final EventPublisher publisher) {
225225
final Object detailsForLogging = getDetailsForLogging(container, replicaProto, datanodeDetails);
226226
if (container == null) {
227-
if (unknownContainerHandleAction.equals(
228-
UNKNOWN_CONTAINER_ACTION_WARN)) {
227+
if (unknownContainerHandleAction == UnknownContainerAction.WARN) {
229228
getLogger().error("CONTAINER_NOT_FOUND for {}", detailsForLogging);
230-
} else if (unknownContainerHandleAction.equals(
231-
UNKNOWN_CONTAINER_ACTION_DELETE)) {
229+
} else if (unknownContainerHandleAction == UnknownContainerAction.DELETE) {
232230
final ContainerID containerId = ContainerID
233231
.valueOf(replicaProto.getContainerID());
234232
deleteReplica(containerId, datanodeDetails, publisher, "CONTAINER_NOT_FOUND", true, detailsForLogging);
@@ -253,26 +251,26 @@ private void processMissingReplicas(final DatanodeDetails datanodeDetails,
253251
final Set<ContainerID> missingReplicas) {
254252
for (ContainerID id : missingReplicas) {
255253
try {
256-
nodeManager.removeContainer(datanodeDetails, id);
254+
getNodeManager().removeContainer(datanodeDetails, id);
257255
} catch (NodeNotFoundException e) {
258-
LOG.warn("Failed to remove container {} from a node which does not " +
259-
"exist {}", id, datanodeDetails, e);
256+
getLogger().warn("Failed to remove missing container {}: datanode {} not found",
257+
id, datanodeDetails, e);
260258
}
261259
try {
262-
containerManager.getContainerReplicas(id).stream()
260+
getContainerManager().getContainerReplicas(id).stream()
263261
.filter(replica -> replica.getDatanodeDetails()
264262
.equals(datanodeDetails)).findFirst()
265263
.ifPresent(replica -> {
266264
try {
267-
containerManager.removeContainerReplica(id, replica);
265+
getContainerManager().removeContainerReplica(id, replica);
268266
} catch (ContainerNotFoundException |
269267
ContainerReplicaNotFoundException ignored) {
270268
// This should not happen, but even if it happens, not an issue
271269
}
272270
});
273271
} catch (ContainerNotFoundException e) {
274-
LOG.warn("Cannot remove container replica, container {} not found.",
275-
id, e);
272+
getLogger().warn("Failed to remove container replica: container {} not found in datanode {}",
273+
id, datanodeDetails, e);
276274
}
277275
}
278276
}

‎hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/IncrementalContainerReportHandler.java

Lines changed: 19 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.io.IOException;
2121
import org.apache.hadoop.hdds.protocol.DatanodeDetails;
2222
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
23+
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State;
2324
import org.apache.hadoop.hdds.scm.container.report.ContainerReportValidator;
2425
import org.apache.hadoop.hdds.scm.exceptions.SCMException;
2526
import org.apache.hadoop.hdds.scm.ha.SCMContext;
@@ -42,27 +43,26 @@ public class IncrementalContainerReportHandler extends
4243
private static final Logger LOG = LoggerFactory.getLogger(
4344
IncrementalContainerReportHandler.class);
4445

45-
private final NodeManager nodeManager;
46-
4746
public IncrementalContainerReportHandler(
4847
final NodeManager nodeManager,
4948
final ContainerManager containerManager,
5049
final SCMContext scmContext) {
51-
super(containerManager, scmContext, LOG);
52-
this.nodeManager = nodeManager;
50+
super(nodeManager, containerManager, scmContext);
51+
}
52+
53+
@Override
54+
protected Logger getLogger() {
55+
return LOG;
5356
}
5457

5558
@Override
5659
public void onMessage(final IncrementalContainerReportFromDatanode report,
5760
final EventPublisher publisher) {
5861
final DatanodeDetails dnFromReport = report.getDatanodeDetails();
59-
if (LOG.isDebugEnabled()) {
60-
LOG.debug("Processing incremental container report from data node {}", dnFromReport);
61-
}
62-
final DatanodeDetails dd = nodeManager.getNode(dnFromReport.getID());
62+
getLogger().debug("Processing incremental container report from datanode {}", dnFromReport);
63+
final DatanodeDetails dd = getNodeManager().getNode(dnFromReport.getID());
6364
if (dd == null) {
64-
LOG.warn("Received container report from unknown datanode {}",
65-
dnFromReport);
65+
getLogger().warn("Datanode not found: {}", dnFromReport);
6666
return;
6767
}
6868

@@ -82,44 +82,38 @@ public void onMessage(final IncrementalContainerReportFromDatanode report,
8282
// Ensure we reuse the same ContainerID instance in containerInfo
8383
id = container.containerID();
8484
} finally {
85-
if (replicaProto.getState().equals(
86-
ContainerReplicaProto.State.DELETED)) {
87-
nodeManager.removeContainer(dd, id);
85+
if (replicaProto.getState() == State.DELETED) {
86+
getNodeManager().removeContainer(dd, id);
8887
} else {
89-
nodeManager.addContainer(dd, id);
88+
getNodeManager().addContainer(dd, id);
9089
}
9190
}
9291
if (ContainerReportValidator.validate(container, dd, replicaProto)) {
9392
processContainerReplica(dd, container, replicaProto, publisher);
9493
}
9594
success = true;
9695
} catch (ContainerNotFoundException e) {
97-
LOG.warn("Container {} not found!", replicaProto.getContainerID());
96+
getLogger().warn("Container {} not found!", replicaProto.getContainerID());
9897
} catch (NodeNotFoundException ex) {
99-
LOG.error("Received ICR from unknown datanode {}",
100-
report.getDatanodeDetails(), ex);
98+
getLogger().error("Datanode not found {}", report.getDatanodeDetails(), ex);
10199
} catch (ContainerReplicaNotFoundException e) {
102-
LOG.warn("Container {} replica not found!",
100+
getLogger().warn("Container {} replica not found!",
103101
replicaProto.getContainerID());
104102
} catch (SCMException ex) {
105103
if (ex.getResult() == SCMException.ResultCodes.SCM_NOT_LEADER) {
106-
LOG.info("Failed to process {} container {}: {}",
104+
getLogger().info("Failed to process {} container {}: {}",
107105
replicaProto.getState(), id, ex.getMessage());
108106
} else {
109-
LOG.error("Exception while processing ICR for container {}",
107+
getLogger().error("Exception while processing ICR for container {}",
110108
replicaProto.getContainerID(), ex);
111109
}
112110
} catch (IOException | InvalidStateTransitionException e) {
113-
LOG.error("Exception while processing ICR for container {}",
111+
getLogger().error("Exception while processing ICR for container {}",
114112
replicaProto.getContainerID(), e);
115113
}
116114
}
117115
}
118116

119117
getContainerManager().notifyContainerReportProcessing(false, success);
120118
}
121-
122-
protected NodeManager getNodeManager() {
123-
return this.nodeManager;
124-
}
125119
}

‎hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestUnknownContainerReport.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ public void testUnknownContainerDeleted() {
115115
OzoneConfiguration conf = new OzoneConfiguration();
116116
conf.set(
117117
ScmConfig.HDDS_SCM_UNKNOWN_CONTAINER_ACTION,
118-
ContainerReportHandler.UNKNOWN_CONTAINER_ACTION_DELETE);
118+
ContainerReportHandler.UnknownContainerAction.DELETE.name());
119119

120120
sendContainerReport(conf);
121121
verify(publisher, times(1)).fireEvent(

‎hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerReportHandler.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,25 @@
2424
import org.apache.hadoop.hdds.scm.node.NodeManager;
2525
import org.apache.hadoop.hdds.scm.server.SCMDatanodeHeartbeatDispatcher.ContainerReportFromDatanode;
2626
import org.apache.hadoop.hdds.server.events.EventPublisher;
27+
import org.slf4j.Logger;
28+
import org.slf4j.LoggerFactory;
2729

2830
/**
2931
* Recon's container report handler.
3032
*/
3133
public class ReconContainerReportHandler extends ContainerReportHandler {
34+
private static final Logger LOG = LoggerFactory.getLogger(ReconContainerReportHandler.class);
3235

3336
public ReconContainerReportHandler(NodeManager nodeManager,
3437
ContainerManager containerManager) {
3538
super(nodeManager, containerManager);
3639
}
3740

41+
@Override
42+
protected Logger getLogger() {
43+
return LOG;
44+
}
45+
3846
@Override
3947
public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
4048
final EventPublisher publisher) {

‎hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconIncrementalContainerReportHandler.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ public ReconIncrementalContainerReportHandler(NodeManager nodeManager,
4747
super(nodeManager, containerManager, scmContext);
4848
}
4949

50+
@Override
51+
protected Logger getLogger() {
52+
return LOG;
53+
}
54+
5055
@Override
5156
public void onMessage(final IncrementalContainerReportFromDatanode report,
5257
final EventPublisher publisher) {

0 commit comments

Comments
 (0)