HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one #5980

jojochuang · 2024-01-11T07:17:06Z

What changes were proposed in this pull request?

Piggyback PutBlock in WriteChunk if the size to be flushed is small.

Please describe your PR in detail:

This PR adds an client side boolean configuration key ozone.client.stream.putblock.piggybacking to merge PutBlock request into WriteChunk, if the chunk size to be flushed to DataNode is small.
Client will detect if the DataNode supports this feature via DatanodeVersion COMBINED_PUTBLOCK_WRITECHUNK_RPC. The feature does not take effect unless DataNode supports it.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit test, integration test. Manually tested on a real cluster.
Hsync throughput and latency has noticeable improvement with this patch.

jojochuang · 2024-01-11T18:05:57Z

@ashishkumar50

jojochuang · 2024-02-08T18:39:26Z

@ashishkumar50

Rebased on HDDS-9752. Client side implementation. Change-Id: I4ff1c3148970edb8820a30dfda607a8aff3f7a52 incomplete server side impl. Change-Id: I3a52c931a4bd60a747ae47737f78df23046d1053 Passed TestHSync Change-Id: Ia70db6af5ed295fdb8fdc55b998a8233dc86c416 Fix bug. When write finishes a full chunk, the chunk is written to DataNode, but the currentChuk is not updated. Therefore, the flusher need to check if the chunk is full or not, and if full, skip writing chunk (because it is already written out). Change-Id: I8685b1894466ec7ebd1bc80c49809523dac4855e Remove debug messages. Change-Id: I6678899c7f03317ad5fbd02298e80dd534a3308a Fix tests and checkstyle Change-Id: I18363e9e3f8b0255adda4fcd40bde0929283256c Fix test Change-Id: Ic9765ee2ec1c15c4558d91380416604dea4f981e Fix counter Change-Id: Ifac56842f36bcca0bfd29d9da7873a98d36134e7 Add a new datanode version. Client do not piggyback PutBlock requests if datanode is not upgraded. Add DN version check Update checkstyle Fix configuration key problem. (cherry picked from commit 8ab02b7) Do not execute PutBlock in WRITE_DATA; only do it in COMMIT_DATA phase. Merge duplicate code. Checkstyle and refactory Fix bug Enable piggyback for the two tests. eckstyle

Change-Id: Ibdb80368b67248d2ca3d44bc6e4c6c756ee41852

Change-Id: I42fd82ccca3e871375acb5256a9da2ff913ab325

Change-Id: I3531fc4fe7d912300b1c02cca48a7e66f468faf0

ashishkumar50

@jojochuang Thanks for the patch, Please find the comments inline.

ashishkumar50 · 2024-03-20T07:43:24Z

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

+        }
+      } else {
+        updateFlushLength();
+        executePutBlock(close, false);


Whether PutBlock required here for small chunk?

This is the typical case (no hflush) where a PutBlock is sent to update metadata after four 1-MB chunks are sent via WriteChunk requests)

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

ashishkumar50 · 2024-03-20T07:56:48Z

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

+        if (config.getIncrementalChunkList()) {
+          // remove any chunks in the containerBlockData list.
+          // since they are sent.
+          containerBlockData.clearChunks();


Do we need to remove last chunk as well here which is updated in updateBlockDataForWriteChunk in case of IncrementalChunkList?

No --
lastChunk is used to calculate checksum in PutBlock (PutBlock contains metadata)
whereas containerBlockData is sent by WriteChunk (WriteChunk contains written data)

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

Change-Id: Iec511ccf2a975c2cac0e76301bb0affb16df56dc

ChenSammi · 2024-03-25T05:27:55Z

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

      KeyValue.newBuilder().setKey(FULL_CHUNK).build();

+  // Flushed chunk less than 100KB will combine PutBlock and WriteChunk in the same request.
+  private static final int SMALL_CHUNK_THRESHOLD = 100 * 1024;


Can we make this configurable?

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

...on/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/ContainerCommandResponseBuilders.java

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java

ChenSammi · 2024-03-25T08:29:34Z

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

          .writeChunk(kvContainer, blockID, chunkInfo, data, dispatcherContext);

+      final boolean isCommit = dispatcherContext.getStage().isCommit();
+      if (isCommit && writeChunk.hasBlock()) {


Shall we check COMBINED_PUTBLOCK_WRITECHUNK_RPC is finalized here?

COMBINED_PUTBLOCK_WRITECHUNK_RPC doesn't have the concept of "finalize". Once the runtime is updated, the datanode version becomes COMBINED_PUTBLOCK_WRITECHUNK_RPC.

which should make sense because this PR does not change on-disk format.

Change-Id: Ic0928f7d57248496dd8596332d90db7312d4ad95

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

Change-Id: I53ed73ca0611cacc387ddd5559f6bbd5d8dc1b17

jojochuang · 2024-03-28T17:44:35Z

Thanks. Updated. Those are good suggestions.

ChenSammi

The last patch LGTM. Thanks @jojochuang , and @ashishkumar50 for the review.

…pache#5980) (cherry picked from commit 91e5d2e)

…pache#5980)

jojochuang changed the title ~~Hdds 9130 rebase on hdds 9752~~ HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one Jan 11, 2024

jojochuang requested a review from ChenSammi January 11, 2024 18:05

jojochuang force-pushed the HDDS-7593 branch from 074b6cc to 1c20d84 Compare January 17, 2024 14:37

jojochuang added the hbase HBase on Ozone support label Jan 23, 2024

jojochuang force-pushed the HDDS-9130_rebase_on_HDDS-9752 branch from 43b8acc to 0a0700b Compare January 24, 2024 00:45

jojochuang force-pushed the HDDS-7593 branch from 76a573a to 1c20d84 Compare February 8, 2024 06:22

jojochuang force-pushed the HDDS-9130_rebase_on_HDDS-9752 branch from 0a0700b to 3f355e5 Compare February 8, 2024 08:29

jojochuang marked this pull request as ready for review February 8, 2024 18:39

jojochuang requested a review from smengcl February 8, 2024 18:39

jojochuang force-pushed the HDDS-9130_rebase_on_HDDS-9752 branch from 3f355e5 to 0a4ef7d Compare March 11, 2024 22:55

jojochuang added 3 commits March 11, 2024 19:44

Fix test error

62565d0

Change-Id: Ibdb80368b67248d2ca3d44bc6e4c6c756ee41852

Remove unused config

d7412f1

Change-Id: I42fd82ccca3e871375acb5256a9da2ff913ab325

Fix test bug; update default mini ozone cluster datanode version.

feaa93f

Change-Id: I3531fc4fe7d912300b1c02cca48a7e66f468faf0

ashishkumar50 reviewed Mar 20, 2024

View reviewed changes

Update patch to address review comments.

a4cfde2

Change-Id: Iec511ccf2a975c2cac0e76301bb0affb16df56dc

ChenSammi reviewed Mar 25, 2024

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java Outdated Show resolved Hide resolved

ChenSammi reviewed Mar 25, 2024

View reviewed changes

...on/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/ContainerCommandResponseBuilders.java Outdated Show resolved Hide resolved

ChenSammi reviewed Mar 25, 2024

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java Outdated Show resolved Hide resolved

ChenSammi reviewed Mar 25, 2024

View reviewed changes

Review comments.

3fdd4d4

Change-Id: Ic0928f7d57248496dd8596332d90db7312d4ad95

smengcl force-pushed the HDDS-7593 branch from 44b1242 to 3fe5cde Compare March 27, 2024 23:29

ChenSammi reviewed Mar 28, 2024

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java Outdated Show resolved Hide resolved

ChenSammi reviewed Mar 28, 2024

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java Outdated Show resolved Hide resolved

Rename methods to make them understandable.

62d60c9

Change-Id: I53ed73ca0611cacc387ddd5559f6bbd5d8dc1b17

ChenSammi approved these changes Apr 1, 2024

View reviewed changes

ChenSammi merged commit 91e5d2e into apache:HDDS-7593 Apr 1, 2024

jojochuang added a commit to jojochuang/ozone that referenced this pull request Apr 8, 2024

HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one (a…

ae3a0e3

…pache#5980) (cherry picked from commit 91e5d2e)

chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024

HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one (a…

4a4fa91

…pache#5980)

chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024

HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one (a…

3a210f3

…pache#5980)

HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one #5980

HDDS-9130. [hsync] Combine WriteData and PutBlock requests into one #5980

Uh oh!

Conversation

jojochuang commented Jan 11, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

jojochuang commented Jan 11, 2024

Uh oh!

jojochuang commented Feb 8, 2024

Uh oh!

ashishkumar50 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

jojochuang commented Mar 28, 2024

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