HDDS-8090. When getBlock from a datanode fails, retry other datanodes. #4357

szetszwo · 2023-03-07T02:08:15Z

What changes were proposed in this pull request?

Similar to HDDS-8024, the client should retry other datanodes if it has failed to getBlock from a datanode.

What is the link to the Apache JIRA

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

How was this patch tested?

This is to fix TestHSync

adoroszlai · 2023-03-07T07:53:46Z

Thanks @szetszwo for continuing work on this. Launched 10x10 run, this time only with TestHSync, which failed 12 times.

jojochuang · 2023-03-10T21:24:23Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

jojochuang · 2023-03-10T21:24:33Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

jojochuang · 2023-03-10T21:40:52Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

My understanding is that client will log the warning for the first and second failure, but the third failure it will simply throw the exception. I think it would be clear if the message is something like "Failed to get Block from " + d + ", will try another DataNode"

szetszwo · 2023-03-12T08:03:40Z

@jojochuang , thanks a lot for review this! Just have pushed a commit to address your comments and to add new tests.

Unfortunately, the cannot completely fix TestHSync. Will continue the work in https://issues.apache.org/jira/browse/HDDS-8146

szetszwo · 2023-03-12T08:07:40Z

@adoroszlai , thanks for the ci.yml for running TestHSync! Will continue using it in https://issues.apache.org/jira/browse/HDDS-8146

adoroszlai · 2023-03-12T09:23:29Z

thanks for the ci.yml for running TestHSync! Will continue using it in https://issues.apache.org/jira/browse/HDDS-8146

👍

Note: I think it's better to create a separate branch for repeated tests, to avoid the need for a revert or force-push at the end. If you tweak the fix, just merge the fix branch into the repeat branch.

Also, since CI runs in forks, too, no need to create PR until the fix is verified by repetitions.

jojochuang

Thanks @szetszwo !
Just a very minor comment otherwise this is goo to go.

jojochuang · 2023-03-14T23:35:11Z

...-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/TestXceiverClientGrpc.java

+  public void testGetBlockRetryAlNodes() {
+    final ArrayList<DatanodeDetails> allDNs = new ArrayList<>(dns);
+    Assertions.assertTrue(allDNs.size() > 1);
+    try (XceiverClientGrpc client = new XceiverClientGrpc(pipeline, conf) {


Unrelated, but looks like the existing tests cases don't close XceiverClientGrpc, potentially leaking resources.

Sure, let me fix the existing problems.

jojochuang · 2023-03-15T00:01:58Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

+    return Collections.singletonList(validator);
+  }
+
+  public static List<CheckedBiFunction> getValidatorList(


IMO a getXXX() method typically implies a O( 1 ) operation. It would be better off to rename it, such as toValidatorList() ?

... a getXXX() method typically implies a O( 1 ) operation. ...

This is not true for TreeMap.get(..), LinkedList.get(..), etc. But I am okay to do the rename.

szetszwo · 2023-03-15T20:45:15Z

@jojochuang , thanks for reviewing and merging this!

@adoroszlai , thanks for the ci hints!

* master: (262 commits) HDDS-8153. Integrate ContainerBalancer with MoveManager (apache#4391) HDDS-8090. When getBlock from a datanode fails, retry other datanodes. (apache#4357) HDDS-8163 Use try-with-resources to ensure close rockdb connection in SstFilteringService (apache#4402) HDDS-8065. Provide GNU long options (apache#4394) HDDS-7930. [addendum] input stream does not refresh expired block token. HDDS-7930. input stream does not refresh expired block token. (apache#4378) HDDS-7740. [Snapshot] Implement SnapshotDeletingService (apache#4244) HDDS-8076. Use container cache in Key listing API. (apache#4346) HDDS-8091. [addendum] Generate list of config tags from ConfigTag enum - Hadoop 3.1 compatibility fix (apache#4374) HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod fails as we approach DST. (apache#4382) HDDS-8151. Support fine grained lifetime for root CA certificate (apache#4386) HDDS-8150. RpcClientTest and ConfigurationSourceTest not run due to naming convention (apache#4388) HDDS-8131. Add Configuration for OM Ratis Log Purge Tuning Parameters. (apache#4371) HDDS-8133. Create ozone sh key checksum command (apache#4375) HDDS-8142. Check if no entries in Block DB for a container on container delete (apache#4379) HDDS-8118. Fail container delete on non empty chunks dir (apache#4367) HDDS-8028. JNI for RocksDB SST Dump tool (apache#4315) HDDS-8129. ContainerStateMachine allows two different tasks with the same container id running in parallel. (apache#4370) HDDS-8119. Remove loosely related AutoCloseable from SendContainerOutputStream (apache#4368) close db connection (apache#4366) ...

ivandika3 · 2024-01-31T10:18:35Z

@szetszwo Sorry for the random comment, but from my understanding we already have a logic to retry other datanodes in XceiverClientGrpc#sendCommandWithRetry.

for (DatanodeDetails dn : datanodeList) {
      try {
        if (LOG.isDebugEnabled()) {
          LOG.debug("Executing command {} on datanode {}",
              processForDebug(request), dn);
        }
        // In case the command gets retried on a 2nd datanode,
        // sendCommandAsyncCall will create a new channel and async stub
        // in case these don't exist for the specific datanode.
        reply.addDatanode(dn);
        responseProto = sendCommandAsync(request, dn).getResponse().get();
        if (validators != null && !validators.isEmpty()) {
          for (Validator validator : validators) {
            validator.accept(request, responseProto);
          }
        }
...

The call trace is ContainerProtocolCalls#getBlock -> XceiverClientGrpc#sendCommand -> XceiverClientGrpc#sendCommandWithTraceIDAndRetry -> XceiverClientGrpc#sendCommandWithRetry

This means that getBlock will be retried 27 times (3 from sendCommandWithRetry assuming RATIS/THREE * 3 from the tryEachDatanode * 3 from BlockInputStream's retry policy)? This is also applied to readChunk #4336.

Please correct me if I'm mistaken. Thanks in advance.

szetszwo · 2024-01-31T19:28:26Z

@ivandika3 , you are right there are multiple levels of retries. It seems the original retries (which are not added here) may not work as expected -- some tests were failing when a datanode cannot return the chunk.

szetszwo mentioned this pull request Mar 7, 2023

HDDS-8024. When readChunk from a datanode fails, retry other datanodes. #4336

Merged

adoroszlai added client datanode labels Mar 7, 2023

jojochuang reviewed Mar 10, 2023

View reviewed changes

szetszwo added 3 commits March 12, 2023 14:38

HDDS-8090. When getBlock from a datanode fails, retry other datanodes.

167466b

Check <= 0.

8498897

Addrssed review comments and added new tests.

caf94f1

szetszwo force-pushed the HDDS-8090 branch from 509a786 to caf94f1 Compare March 12, 2023 07:53

Fix checkstyle.

40cee9d

szetszwo requested a review from jojochuang March 14, 2023 02:31

jojochuang reviewed Mar 15, 2023

View reviewed changes

Address review comments.

77fc54a

jojochuang approved these changes Mar 15, 2023

View reviewed changes

jojochuang merged commit 2db7dda into apache:master Mar 15, 2023

HDDS-8090. When getBlock from a datanode fails, retry other datanodes. #4357

HDDS-8090. When getBlock from a datanode fails, retry other datanodes. #4357

Uh oh!

Conversation

szetszwo commented Mar 7, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai commented Mar 7, 2023

Uh oh!

jojochuang Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

jojochuang Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

jojochuang Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Mar 12, 2023

Uh oh!

szetszwo commented Mar 12, 2023

Uh oh!

adoroszlai commented Mar 12, 2023

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

jojochuang Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

szetszwo Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

jojochuang Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

szetszwo Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Mar 15, 2023

Uh oh!

ivandika3 commented Jan 31, 2024

Uh oh!

szetszwo commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants