szetszwo commented on code in PR #6886:
URL: https://github.com/apache/ozone/pull/6886#discussion_r1685328298
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java:
##########
@@ -655,41 +659,45 @@ public void execute(ExecutorService service, long time,
TimeUnit unit)
// we called stop DatanodeStateMachine, this sets state to SHUTDOWN, and
// there is a chance of getting task as null.
if (task != null) {
Review Comment:
Let's change the if-statement to return instead of add a new indentation.
```java
@@ -654,7 +658,11 @@ public void execute(ExecutorService service, long time,
TimeUnit unit)
// Adding not null check, in a case where datanode is still starting
up, but
// we called stop DatanodeStateMachine, this sets state to SHUTDOWN, and
// there is a chance of getting task as null.
- if (task != null) {
+ if (task == null) {
+ return;
+ }
+
+ try {
if (this.isEntering()) {
task.onEnter();
}
@@ -691,6 +699,8 @@ public void execute(ExecutorService service, long time,
TimeUnit unit)
// that we can terminate the datanode.
setShutdownOnError();
}
+ } finally {
+ task.clear();
}
}
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java:
##########
@@ -173,11 +118,32 @@ public void
setExecutorCompletionService(ExecutorCompletionService e) {
private Callable<EndPointStates> getEndPointTask(
EndpointStateMachine endpoint) {
- if (endpointTasks.containsKey(endpoint)) {
- return endpointTasks.get(endpoint).get(endpoint.getState());
- } else {
- throw new IllegalArgumentException("Illegal endpoint: " + endpoint);
+ Callable<EndPointStates> endpointTask = null;
+ for (EndpointStateMachine endpointStateMachine :
connectionManager.getValues()) {
+ if
(endpointStateMachine.getAddressString().equals(endpoint.getAddressString())) {
+ if (endpoint.getState().getValue() ==
EndPointStates.GETVERSION.getValue()) {
+ endpointTask = new VersionEndpointTask(endpoint, conf,
+ context.getParent().getContainer());
+ } else if (endpoint.getState().getValue() ==
EndPointStates.REGISTER.getValue()) {
+ endpointTask = RegisterEndpointTask.newBuilder()
+ .setConfig(conf)
+ .setEndpointStateMachine(endpoint)
+ .setContext(context)
+ .setDatanodeDetails(context.getParent().getDatanodeDetails())
+ .setOzoneContainer(context.getParent().getContainer())
+ .build();
+ } else if (endpoint.getState().getValue() ==
EndPointStates.HEARTBEAT.getValue()) {
+ endpointTask = HeartbeatEndpointTask.newBuilder()
+ .setConfig(conf)
+ .setEndpointStateMachine(endpoint)
+ .setDatanodeDetails(context.getParent().getDatanodeDetails())
+ .setContext(context)
+ .build();
+ }
Review Comment:
What is the reason to create an `endpointTask` each time instead of using
the `endpointTasks` map?
--
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]
