HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout. #4913

xichen01 · 2023-06-16T07:28:41Z

What changes were proposed in this pull request?

Some performance issues are mentioned here. https://issues.apache.org/jira/browse/HDDS-8865 so in this PR:
We Make DN DeleteBlocksCommandHandler wait for the lock to timeout. This way, no other transaction will be blocked because the lock cannot be acquired, and for locks that time out, we can retry once on the DN if can not acquired the lock in specified time, transaction will failed and the SCM can resent it after.

What is the link to the Apache JIRA

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

How was this patch tested?

unit test

adoroszlai

Thanks @xichen01 for the patch. I haven't checked in detail, but would like to point out some minor issues noticed.

adoroszlai · 2023-06-19T12:39:02Z

...adoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java

+@RunWith(Parameterized.class)
+@Timeout(300)


Nit: @Timeout is from JUnit5, all other test annotations are from JUnit4. I don't think timeout is enforced with this setup.

adoroszlai · 2023-06-19T12:40:49Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeStateMachine.java

            conf, dnConf.getBlockDeleteThreads(),
-            dnConf.getBlockDeleteQueueLimit()))
+            dnConf.getBlockDeleteQueueLimit(),
+            dnConf.getBlockDeleteCommandHandleLockTimeoutMs()))


Nit: pass DatanodeConfiguration to DeleteBlocksCommandHandler instead of individual values.

adoroszlai · 2023-06-19T12:42:37Z

.../main/java/org/apache/hadoop/ozone/container/common/helpers/BlockDeletingServiceMetrics.java

+  @Metric(about = "The total number of transactions witch was failed " +
+      "cause by wait Container lock timeout.")


Typo: witch -> which

# Conflicts: # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/BlockDeletingServiceMetrics.java # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java

xichen01 · 2023-06-23T17:34:28Z

@adoroszlai PTAL. Thanks.

adoroszlai

Thanks @xichen01 for updating the patch.

Note: there is a findbugs failure:

M D UrF: Unread public/protected field: org.apache.hadoop.ozone.container.common.statemachine.commandhandler.TestDeleteBlocksCommandHandler.testTimeout  At TestDeleteBlocksCommandHandler.java:[line 80]

https://github.com/xichen01/ozone/actions/runs/5358921181/jobs/9721855321#step:6:2324

Once you get a clean run, please ask @sumitagrawl for review.

adoroszlai · 2023-06-28T12:23:28Z

...adoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java

+  @Parameterized.Parameters(name = "{index}: Schema Version {0}")
+  public static Collection<Object[]> data() {
+    return Arrays.asList(new Object[][]{
+        {SCHEMA_V1},
+        {SCHEMA_V2},
+        {SCHEMA_V3},
+    });
+  }


You can use ContainerTestVersionInfo to parameterize by schema version and layout version.

xichen01 · 2023-07-06T03:40:58Z

@sumitagrawl PTAL. Thanks

xichen01 · 2023-07-13T15:21:22Z

@adoroszlai PTAL, Thanks

ChenSammi · 2023-09-20T03:59:30Z

...c/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java

+   * after waiting timeout, the hold time spent waiting for the lock
+   * is not greater than the time spent operating RocksDB
+   */
+  @Config(key = "block.delete.command.handle.lock.timeout",


Suggest change the property name from block.delete.command.handle.lock.timeout ->
block.delete.max.lock.wait.timeout

@xichen01 , is the slow RocksDB open a major cause behind this patch? I have one question, it takes 200ms to open a rocksdb, you set this timeout to 100ms, and retry once in the block deletion logic, why not set the default value to 200ms, with a simpler handler logic?

Will this property be a reconfigurable property?

This comment may have caused some misunderstanding, I'll modify it.

The retry does not happen immediately, it waits until all DeleteBlockTransactions have been processed, so the retry may happen after a few seconds or tens of seconds.

Slow RocksDB open (Schema V2 only), rebalancer, BlockDeletingService, etc, may cause here cannot get the lock, retry to increase the chance of success.

@xichen01 , thanks for the further info.

ChenSammi · 2023-09-20T04:00:22Z

.../main/java/org/apache/hadoop/ozone/container/common/helpers/BlockDeletingServiceMetrics.java

  @Metric(about = "The total number of Container chosen to be deleted.")
  private MutableGaugeLong totalContainerChosenCount;

+  @Metric(about = "The total number of transactions which was failed " +


which was failed cause by wait Container lock timeout -> which failed due to container lock wait timeout

ChenSammi · 2023-09-20T06:16:32Z

...he/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java

                  "containerType {}", containerType);
        }
-      } catch (IOException e) {
+      } catch (IOException | InterruptedException e) {


Please call Thread.currentThread().interrupt() if it's an InterruptedException.

ChenSammi · 2023-09-20T06:39:47Z

...he/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java

-      int newDeletionBlocks, long txnID)
+      KeyValueContainerData containerData, DeletedBlocksTransaction delTX)
      throws IOException {
+    int newDeletionBlocks = 0;


Could you do some refactor on markBlocksForDeletionTransaction too? This newDeletionBlocks is always 0. It's better to be declared in markBlocksForDeletionTransaction.

Already modified.

ChenSammi · 2023-09-22T10:31:43Z

The failed TestOzoneFileSystem.testListStatusOnKeyNameContainDelimiter is being addressed by HDDS-8877 #5093

https://github.com/apache/ozone/actions/runs/6269541505/job/17030065862
https://github.com/apache/ozone/actions/runs/6269541505/job/17026517710

ChenSammi

Thanks @xichen01 for the contribution, @adoroszlai for the review.

…e lock can timeout. (apache#4913) (cherry picked from commit 5b5d352)

Make DN DeleteBlocksCommandHandler wait for the lock can timeout.

426b3cf

adoroszlai requested review from sumitagrawl and symious June 19, 2023 12:42

adoroszlai reviewed Jun 19, 2023

View reviewed changes

xichen01 added 2 commits June 20, 2023 00:42

Typo; Use junit4 timeout; Change DeleteBlocksCommandHandler parameters

6a8b74e

adoroszlai reviewed Jun 28, 2023

View reviewed changes

xichen01 added 3 commits June 29, 2023 01:46

Fix findbugs

fd0105f

Using ContainerTestVersionInfo to parameterize test

2b8e4ea

Exclude findbugs URF

9fa10cb

adoroszlai requested review from ChenSammi and aryangupta1998 July 17, 2023 06:59

xichen01 requested a review from adoroszlai July 28, 2023 16:21

ChenSammi reviewed Sep 20, 2023

View reviewed changes

xichen01 and others added 3 commits September 22, 2023 00:51

Modify comments; Modify markBlocksForDeletionTransaction

c1ddf5d

Update indent in DeleteBlocksCommandHandler.java

eeb9f8e

Update indent in TestDeleteBlocksCommandHandler.java

7ede747

ChenSammi approved these changes Sep 22, 2023

View reviewed changes

ChenSammi merged commit 5b5d352 into apache:master Sep 22, 2023

swamirishi pushed a commit to swamirishi/ozone that referenced this pull request Dec 3, 2025

CDPD-57782. HDDS-8869. Make DN DeleteBlocksCommandHandler wait for th…

3028378

…e lock can timeout. (apache#4913) (cherry picked from commit 5b5d352)

		@Metric(about = "The total number of transactions witch was failed " +
		"cause by wait Container lock timeout.")

		@RunWith(Parameterized.class)
		@Timeout(300)

HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout. #4913

HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout. #4913

Uh oh!

Conversation

xichen01 commented Jun 16, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xichen01 commented Jun 23, 2023

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xichen01 commented Jul 6, 2023

Uh oh!

xichen01 commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ChenSammi commented Sep 22, 2023 •

edited

Loading