HDDS-11083. Avoid duplicate creation of RunningDatanodeState #6886

jianghuazhu · 2024-06-29T07:12:15Z

What changes were proposed in this pull request?

When DN is running, the RunningDatanodeState is created every time the heartbeat is sent to the SCM. This operation is very frequent and should be reused as much as possible.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11083

How was this patch tested?

Make sure that your unit tests pass.

jianghuazhu · 2024-06-29T07:39:45Z

ci : https://github.com/jianghuazhu/ozone/actions/runs/9717049941
In CI/CD, several unit tests failed:
TestECContainerRecovery
TestStorageContainerManager
TestReconAndAdminContainerCLI
TestDecommissionAndMaintenance

It seems they all failed for the same reason.

This needs to be addressed in another jira, and I'll create a new issue later.

@adoroszlai @hemantk-12 @errose28 , can you help review this PR.
thank you.

jianghuazhu · 2024-06-29T07:42:48Z

ci : https://github.com/jianghuazhu/ozone/actions/runs/9717049941
In CI/CD, several unit tests failed:
TestECContainerRecovery
TestStorageContainerManager
TestReconAndAdminContainerCLI
TestDecommissionAndMaintenance

It seems they all failed for the same reason.

This needs to be addressed in another jira, and I'll create a new issue later.

@adoroszlai @hemantk-12 @errose28 , can you help review this PR.
thank you.

adoroszlai · 2024-06-29T11:48:48Z

n CI/CD, several unit tests failed:
TestECContainerRecovery
TestStorageContainerManager
TestReconAndAdminContainerCLI
TestDecommissionAndMaintenance

This needs to be addressed in another jira

These tests are not encountering BindException without this change, so why would you address in another Jira instead of this one?

jianghuazhu · 2024-07-09T04:08:18Z

Thanks @adoroszlai for the comments and review.
I'm trying to solve these problems.

jianghuazhu · 2024-07-17T13:39:04Z

ci：https://github.com/jianghuazhu/ozone/actions/runs/9974167102
I have updated it and all CI passes.
Can you review it again, @adoroszlai ?
Thanks.

adoroszlai · 2024-07-17T13:45:58Z

...ner-service/src/main/java/org/apache/hadoop/ozone/container/common/states/DatanodeState.java

+  /**
+   * Clean up some resources.
+   */
+  void clear();


Please add default no-op implementation.

adoroszlai · 2024-07-17T13:48:11Z

...ce/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/TestStateContext.java

+          @Override
+          public void clear() {
+
+          }


No change needed in this file after adding default no-op implementation.

adoroszlai · 2024-07-17T13:48:25Z

...rc/main/java/org/apache/hadoop/ozone/container/common/states/datanode/InitDatanodeState.java

+  @Override
+  public void clear() {
+
+  }


No change needed in this file after adding default no-op implementation.

I have updated it.

jianghuazhu · 2024-07-19T09:44:25Z

Do you think there are other places in this PR that need to be improved? If so, I will work hard to improve it. @adoroszlai @hemantk-12

adoroszlai · 2024-07-19T09:51:37Z

@jianghuazhu Thanks for working on this. Sorry, I have no time to review it, please ask others.

jianghuazhu · 2024-07-19T11:04:58Z

@jianghuazhu Thanks for working on this. Sorry, I have no time to review it, please ask others.
Thanks @adoroszlai .

szetszwo

@jianghuazhu , thanks for working on this! Please see the comment/question inlined.

szetszwo · 2024-07-20T09:00:20Z

...ervice/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) {


Let's change the if-statement to return instead of add a new indentation.

@@ -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(); } }

I'll update later.

szetszwo · 2024-07-20T09:01:52Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+    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();
+        }


What is the reason to create an endpointTask each time instead of using the endpointTasks map?

Thanks @szetszwo .
There are two reasons for this:

If endpointTasks is kept and initialized once, unfortunately, some CI fail.

If endpointTasks is created each time execute is executed, a VersionEndpointTask, RegisterEndpointTask, and HeartbeatEndpointTask are created. However, HeartbeatEndpointTask is more often used, so it can be created based on the endpoint state, which is less expensive.

szetszwo

@jianghuazhu , thanks for the updates and the explanation! Please see the comments inlined.

szetszwo · 2024-07-21T07:26:41Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+    for (EndpointStateMachine endpointStateMachine : connectionManager.getValues()) {
+      if (endpointStateMachine.getAddressString().equals(endpoint.getAddressString())) {


This for loop and the if-statement are not needed since endpointStateMachine is not used to build the task. Also, we already has it in the execute(..) method.

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

Line 142 in a6b3392

for (EndpointStateMachine endpoint : connectionManager.getValues()) {

szetszwo · 2024-07-21T07:26:56Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+    Callable<EndPointStates> endpointTask = null;
+    for (EndpointStateMachine endpointStateMachine : connectionManager.getValues()) {
+      if (endpointStateMachine.getAddressString().equals(endpoint.getAddressString())) {
+        if (endpoint.getState().getValue() == EndPointStates.GETVERSION.getValue()) {


Use switch-case instead.

szetszwo · 2024-07-21T07:28:05Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

  private Callable<EndPointStates> getEndPointTask(
      EndpointStateMachine endpoint) {


Let's rename it to buildEndPointTask.

private Callable<EndPointStates> buildEndPointTask(EndpointStateMachine endpoint) { switch (endpoint.getState()) { case GETVERSION: return new VersionEndpointTask(endpoint, conf, context.getParent().getContainer()); case REGISTER: return RegisterEndpointTask.newBuilder() .setConfig(conf) .setEndpointStateMachine(endpoint) .setContext(context) .setDatanodeDetails(context.getParent().getDatanodeDetails()) .setOzoneContainer(context.getParent().getContainer()) .build(); case HEARTBEAT: return HeartbeatEndpointTask.newBuilder() .setConfig(conf) .setEndpointStateMachine(endpoint) .setDatanodeDetails(context.getParent().getDatanodeDetails()) .setContext(context) .build(); default: return null; } }

Thanks @szetszwo .
I'll update soon.

I've updated it, @szetszwo .

szetszwo

+1 the change looks good.

szetszwo · 2024-07-24T17:16:09Z

Just have restarted the failed tests.

jianghuazhu · 2024-07-25T02:27:09Z

Just have restarted the failed tests.
Thanks @szetszwo .

adoroszlai · 2024-07-26T18:52:28Z

Thanks @jianghuazhu for the patch, @szetszwo for the review.

adoroszlai marked this pull request as draft June 29, 2024 11:47

jianghuazhu force-pushed the HDDS-11083 branch 4 times, most recently from 750d650 to ac48935 Compare July 4, 2024 03:08

jianghuazhu force-pushed the HDDS-11083 branch 4 times, most recently from 30126e0 to 61ad24c Compare July 17, 2024 10:01

HDDS-11083. Avoid duplicate creation of RunningDatanodeState

09f7f28

jianghuazhu force-pushed the HDDS-11083 branch from 61ad24c to 09f7f28 Compare July 17, 2024 12:26

adoroszlai reviewed Jul 17, 2024

View reviewed changes

Add default method

e872950

adoroszlai requested review from szetszwo and nandakumar131 July 19, 2024 10:57

szetszwo reviewed Jul 20, 2024

View reviewed changes

Update StateContext.

a484992

szetszwo reviewed Jul 21, 2024

View reviewed changes

szetszwo marked this pull request as ready for review July 21, 2024 07:29

Update RunningDatanodeState.

d9c541f

szetszwo approved these changes Jul 22, 2024

View reviewed changes

szetszwo merged commit c760804 into apache:master Jul 25, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-11083. Avoid duplicate creation of RunningDatanodeState #6886

HDDS-11083. Avoid duplicate creation of RunningDatanodeState #6886

jianghuazhu commented Jun 29, 2024

jianghuazhu commented Jun 29, 2024

jianghuazhu commented Jun 29, 2024

adoroszlai commented Jun 29, 2024

jianghuazhu commented Jul 9, 2024

jianghuazhu commented Jul 17, 2024

adoroszlai Jul 17, 2024

adoroszlai Jul 17, 2024

adoroszlai Jul 17, 2024

jianghuazhu Jul 17, 2024

jianghuazhu commented Jul 19, 2024

adoroszlai commented Jul 19, 2024

jianghuazhu commented Jul 19, 2024

szetszwo left a comment

szetszwo Jul 20, 2024

jianghuazhu Jul 20, 2024

szetszwo Jul 20, 2024

jianghuazhu Jul 20, 2024

szetszwo left a comment

szetszwo Jul 21, 2024 •

edited

Loading

szetszwo Jul 21, 2024

szetszwo Jul 21, 2024

jianghuazhu Jul 21, 2024

jianghuazhu Jul 22, 2024

szetszwo left a comment

szetszwo commented Jul 24, 2024

jianghuazhu commented Jul 25, 2024

adoroszlai commented Jul 26, 2024

		for (EndpointStateMachine endpointStateMachine : connectionManager.getValues()) {
		if (endpointStateMachine.getAddressString().equals(endpoint.getAddressString())) {

		private Callable<EndPointStates> getEndPointTask(
		EndpointStateMachine endpoint) {

HDDS-11083. Avoid duplicate creation of RunningDatanodeState #6886

HDDS-11083. Avoid duplicate creation of RunningDatanodeState #6886

Conversation

jianghuazhu commented Jun 29, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

jianghuazhu commented Jun 29, 2024

jianghuazhu commented Jun 29, 2024

adoroszlai commented Jun 29, 2024

jianghuazhu commented Jul 9, 2024

jianghuazhu commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianghuazhu commented Jul 19, 2024

adoroszlai commented Jul 19, 2024

jianghuazhu commented Jul 19, 2024

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo commented Jul 24, 2024

jianghuazhu commented Jul 25, 2024

adoroszlai commented Jul 26, 2024

szetszwo Jul 21, 2024 •

edited

Loading