HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of the stream. #3229

szetszwo · 2022-03-23T17:05:41Z

What changes were proposed in this pull request?

Retain the buffers (using RATIS-1556) so that the PutBlockRequest can be deserialized when the stream is closed.

What is the link to the Apache JIRA

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

How was this patch tested?

New unit tests.

guohao-rosicky · 2022-03-24T11:04:58Z

Of course, Ozone may choose to add a header for each call but it will add more overhead and need more code changes.

This header is actually included in every ratis Stream request.

org.apache.ratis.netty.NettyDataStreamUtils#encodeDataStreamRequestHeader
https://github.com/apache/ratis/blob/master/ratis-netty/src/main/java/org/apache/ratis/netty/NettyDataStreamUtils.java#L90

Can we use it?

szetszwo · 2022-03-24T12:55:12Z

This header is actually included in every ratis Stream request. ...

@guohao-rosicky , that is Ratis header but not Ozone header. Later on, Ratis can buffer the small requests at the client side before sending it out for performance improvement; see https://issues.apache.org/jira/browse/RATIS-1157 . That's the reason that we cannot depend on individual packet sizes or empty packets.

As an analogy, we use TCP in streaming. We may not be able to use TCP header in our code.

captainzmc · 2022-03-26T05:10:53Z

Thanks @szetszwo update this PR. Let me test this PR first and then we can continue the review.

szetszwo · 2022-03-26T14:57:43Z

@captainzmc , sure, please test this. Thanks a lot!

captainzmc · 2022-03-27T11:38:41Z

...src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/KeyValueStreamDataChannel.java

Hi @szetszwo. I just tested the PR and found that the previous test case did not run successfully. There are a lot of AbstractMethodError found. Here is datanode log. Can you confirm that?

@captainzmc , I guess you somehow were using some old version of Ratis/Ozone since you got AbstractMethodError. Please try deleting the maven cache and then rebuild everything.

This change should work since it already has passed all the checks in https://github.com/apache/ozone/runs/5687893931

Oops, my mistake, I didn't replace all the jars. Let me recompile and retest.

I recompiled the code and the tests were fine.

guohao-rosicky · 2022-03-28T02:40:26Z

Hi, @szetszwo and @captainzmc

The current code does not handle small files separately:

Current process:

init stream
write data
put block with stream
Close and send put block proto's length

Is it necessary to continue #2860 after this PR is merged?

Because there are only two steps in its transmission:

init stream
close and put small file proto (data + put block)

captainzmc · 2022-03-28T04:05:13Z

Is it necessary to continue #2860 after this PR is merged?

Hi @guohao-rosicky. I think writing small file using Streaming needs to continue separately in #2860. @szetszwo Any suggestion here?

szetszwo · 2022-03-28T08:27:21Z

init stream

write data

put block with stream

Close and send put block proto's length

If we buffer everything before sending it out, this is actually is a single step for small files in the transmission. Indeed, write small file should only have one transmission. Why it needs to take two transmissions to send a small file?

@captainzmc , @guohao-rosicky , I am okay to continue working on #2860. As mentioned above, we should just buffer everything in the client side.

captainzmc · 2022-03-28T08:57:35Z

Thanks @szetszwo. There are some conflicts in the current code. I think after resolving the conflict, we can merge this PR first.

guohao-rosicky · 2022-03-28T08:58:54Z

I am okay to continue working on #2860. As mentioned above, we should just buffer everything in the client side.

Thanks @szetszwo Can You take over the job?

… the stream.

captainzmc

+1 Thanks @szetszwo, Let merge this.

szetszwo · 2022-03-28T12:47:06Z

@captainzmc , thanks a lot for reviewing and merging this!

szetszwo · 2022-03-28T12:51:18Z

Thanks @szetszwo Can You take over the job?

Sure.

szetszwo · 2022-03-28T12:51:54Z

@captainzmc , I actually like to run some benchmarks. Could you show me the details steps (including setting up a cluster)?

captainzmc · 2022-03-28T13:03:36Z

@captainzmc , I actually like to run some benchmarks. Could you show me the details steps (including setting up a cluster)?

@szetszwo Sure, I will prepare a test document and update it here.

captainzmc · 2022-03-28T14:26:08Z

@captainzmc , I actually like to run some benchmarks. Could you show me the details steps (including setting up a cluster)?

Hi @szetszwo here is my test doc. It contains my test results and procedures. I recommend using Freon instead of my test code, because it's easier. Please leave me a message if you have any questions.

https://docs.google.com/document/d/1byRoqHJXMIVaJWVpIX_vtwMzK8PO6CQlNlanlanffXQ/edit#heading=h.tflxcwmtnab

szetszwo · 2022-03-28T14:48:00Z

@captainzmc , thanks a lot for sharing the test details! Definitely will try it.

captainzmc · 2022-04-19T12:20:18Z

@captainzmc , thanks a lot for sharing the test details! Definitely will try it.

Hi @szetszwo, Any progress on the streaming tests? Are there performance and stability issues? We can keep discuss if there are any questions.

szetszwo · 2022-04-19T13:36:09Z

@captainzmc , I tried running some single client benchmarks. However, the ASYNC performance is better than STREAM Found that we have not called RaftServerConfigKeys.DataStream.setAsyncWriteThreadPoolSize. I temporarily have the following change:

+++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
@@ -237,6 +237,8 @@ private void setUpRatisStream(RaftProperties properties) {
             .getStreamRequestThreads();
     RaftServerConfigKeys.DataStream.setAsyncRequestThreadPoolSize(properties,
         dataStreamAsyncRequestThreadPoolSize);
+    RaftServerConfigKeys.DataStream.setAsyncWriteThreadPoolSize(properties,
+        dataStreamAsyncRequestThreadPoolSize);
     int dataStreamClientPoolSize =
         conf.getObject(DatanodeRatisServerConfig.class)
             .getClientPoolSize();

captainzmc · 2022-04-20T12:20:30Z

@captainzmc , I tried running some single client benchmarks. However, the ASYNC performance is better than STREAM Found that we have not called RaftServerConfigKeys.DataStream.setAsyncWriteThreadPoolSize. I temporarily have the following change:

+++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
@@ -237,6 +237,8 @@ private void setUpRatisStream(RaftProperties properties) {
             .getStreamRequestThreads();
     RaftServerConfigKeys.DataStream.setAsyncRequestThreadPoolSize(properties,
         dataStreamAsyncRequestThreadPoolSize);
+    RaftServerConfigKeys.DataStream.setAsyncWriteThreadPoolSize(properties,
+        dataStreamAsyncRequestThreadPoolSize);
     int dataStreamClientPoolSize =
         conf.getObject(DatanodeRatisServerConfig.class)
             .getClientPoolSize();

@szetszwo, Our previous test results are similar, streaming performance is not as good as async. However, the bottleneck here is probably not WriteThreadPool. In our tests we looked at the VM monitor, and WriteThreadPool and RequestThreadPool were mostly idle. So we think the problem might be Netty.

szetszwo · 2022-04-20T15:34:30Z

... So we think the problem might be Netty.

@captainzmc , ASYNC also uses Netty (gRPC + Netty). If the slowness in STREAM is due to Netty, then it should just be a configuration problem.

BTW, we did not see similar problem in FileStore benchmarks we did earlier.

captainzmc · 2022-04-21T03:38:11Z

If the slowness in STREAM is due to Netty, then it should just be a configuration problem.

Agree，Maybe there are some Netty configurations that we haven't noticed.

BTW, we did not see similar problem in FileStore benchmarks we did earlier.

@szetszwo Yes, I remember that. runzhiwang and I did the FileStore test together. FileStore performed well at the time. But recently I noticed that runzhiwang commented a line of code in the Filestore test code. There was no logic for startTransaction in the previous Filestore test. This line is required in Ozone code. That's as different as I can think of so far.

szetszwo · 2022-04-21T05:39:24Z

There was no logic for startTransaction in the previous Filestore test. This line is required in Ozone code.

I see. Then, if we move the startTransaction to the client slide, it may help a lot. Let me try.

… the stream. (apache#3229)

szetszwo · 2022-05-13T04:00:42Z

@captainzmc , @guohao-rosicky , after some tests, I found that the current client Stream code is slow. The Stream code was mainly copied from the Async code. The flush and watch handling in Async seems slowing down Streaming since they call wait().

captainzmc · 2022-05-13T05:47:20Z

@captainzmc , @guohao-rosicky , after some tests, I found that the current client Stream code is slow. The Stream code was mainly copied from the Async code. The flush and watch handling in Async seems slowing down Streaming since they call wait().

Thanks @szetszwo for the test, the performance problem may be not easy to solve. I wonder if we can continue with HDDS-4474 (WriteSmallFile) and HDDS-5869 (S3Gateway)? The streaming will be almost complete after these two are completed.
After finish HDDS-4474 and HDDS-5869, we can start the Merge Master. More work can continue on the Master, and there will have more people help optimize. It would be best if streaming can add into ozone 1.3.0 release.

szetszwo · 2022-05-13T07:02:50Z

@captainzmc , I am fine to continue working to these two JIRAs.

For HDDS-4474 (WriteSmallFile), I thought @guohao-rosicky was going to work on RATIS-1157 ?

For HDDS-5869 (S3Gateway), it seems that the pull request was not able to pass all the tests yet.

… the stream. (#3229)

guohao-rosicky · 2022-05-13T07:43:48Z

Hi, @szetszwo

For HDDS-5869 (S3Gateway), it seems that the pull request was not able to pass all the tests yet.

For now, I will focus on HDDS-5869 (S3Gateway), which will help us merge HDDS-4454 into master and add ozone 1.3.0 release.

For HDDS-4474 (WriteSmallFile), I thought @guohao-rosicky was going to work on RATIS-1157 ?

Ratis-1157 and HDDS-4474 (WriteSmallFile), I think can continue to optimize after HDDS-4454 merged into master.

captainzmc · 2022-05-13T08:47:53Z

Ratis-1157 and HDDS-4474 (WriteSmallFile), I think can continue to optimize after HDDS-4454 merged into master.

Thanks @guohao-rosicky. Agree. Ratis2.3.0 is currently in release and will not contain ratis-1157. Hdds-4474 WriteSmallFile optimization we can improve after merged into master. @szetszwo, Any suggestions here?

… the stream. (#3229)

szetszwo · 2022-06-07T19:13:13Z

@captainzmc , @guohao-rosicky , I find out why Ozone-Streaming is slow -- the watch requests slows it down. Will submit a pr.

… the stream. (#3229)

… the stream. (apache#3229)

… the stream. (#3229) (cherry picked from commit d72a8a0) (cherry picked from commit 1dddbec2524ca3463aa6140f33c6f16f0468fce8)

… the stream. (#3229) (cherry picked from commit d72a8a0) (cherry picked from commit 1dddbec2524ca3463aa6140f33c6f16f0468fce8) (cherry picked from commit f2d574c)

… the stream. (#3229) (cherry picked from commit d0407c1)

… the stream. (apache#3229) (cherry picked from commit d0407c1)

guohao-rosicky · 2023-06-06T02:21:40Z

hi, @szetszwo . If the client exits abnormally, the stream close is not called; Whether can cause org.apache.hadoop.ozone.container.keyvalue.impl.KeyValueStreamDataChannel.Buffers in memory is not released.

see:
https://github.com/apache/ozone/blob/master/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/KeyValueStreamDataChannel.java#L62

https://github.com/apache/ozone/blob/master/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/KeyValueStreamDataChannel.java#L199

szetszwo · 2023-06-06T03:59:10Z

@guohao-rosicky , thanks a lot for pointing it out! Would you like to submit a pull request for fixing it? Or I can do it.

guohao-rosicky · 2023-06-06T05:40:47Z

@guohao-rosicky , thanks a lot for pointing it out! Would you like to submit a pull request for fixing it? Or I can do it.
@szetszwo
Thank you for your attention. You can fix it, that's very important to me.
Can you submit a jira? I will continue to follow this jira.

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

szetszwo requested a review from captainzmc March 23, 2022 17:05

szetszwo mentioned this pull request Mar 24, 2022

HDDS-6137. [Ozone-Streaming] Refactor KeyDataStreamOutput. #3195

Merged

szetszwo force-pushed the HDDS-4454 branch from eaef7ea to b9d36ba Compare March 24, 2022 12:41

szetszwo force-pushed the HDDS-6500 branch from 0b621f8 to a00fef8 Compare March 24, 2022 15:14

captainzmc reviewed Mar 27, 2022

View reviewed changes

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

b6fb028

… the stream.

szetszwo force-pushed the HDDS-6500 branch from 71a3187 to b6fb028 Compare March 28, 2022 09:47

captainzmc approved these changes Mar 28, 2022

View reviewed changes

captainzmc merged commit 30c9cc8 into apache:HDDS-4454 Mar 28, 2022

szetszwo added a commit to szetszwo/ozone that referenced this pull request May 6, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

246b0a6

… the stream. (apache#3229)

szetszwo added a commit that referenced this pull request May 13, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

1ee1a9e

… the stream. (#3229)

szetszwo added a commit that referenced this pull request May 24, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

f5175db

… the stream. (#3229)

szetszwo added a commit that referenced this pull request Jun 9, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

96d5eb0

… the stream. (#3229)

captainzmc pushed a commit to captainzmc/hadoop-ozone that referenced this pull request Jul 4, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

d72a8a0

… the stream. (apache#3229)

szetszwo added a commit that referenced this pull request Oct 25, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

f2d574c

… the stream. (#3229) (cherry picked from commit d72a8a0) (cherry picked from commit 1dddbec2524ca3463aa6140f33c6f16f0468fce8)

szetszwo added a commit that referenced this pull request Dec 1, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

20e96d8

… the stream. (#3229) (cherry picked from commit d0407c1)

szetszwo added a commit that referenced this pull request Dec 16, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

6a63842

… the stream. (#3229) (cherry picked from commit d0407c1)

nishitpatira pushed a commit to nishitpatira/ozone that referenced this pull request Dec 16, 2022

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of…

1abb334

… the stream. (apache#3229) (cherry picked from commit d0407c1)

szetszwo mentioned this pull request Jun 7, 2023

RATIS-1847. Stream has memory leak. apache/ratis#884

Merged

ivandika3 reviewed Jan 2, 2025

View reviewed changes

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

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of the stream. #3229

HDDS-6500. [Ozone-Streaming] Buffer the PutBlockRequest at the end of the stream. #3229

Uh oh!

Conversation

szetszwo commented Mar 23, 2022

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

guohao-rosicky commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Mar 24, 2022

Uh oh!

captainzmc commented Mar 26, 2022

Uh oh!

szetszwo commented Mar 26, 2022

Uh oh!

captainzmc Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

captainzmc Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

captainzmc Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

guohao-rosicky commented Mar 28, 2022

Uh oh!

captainzmc commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Mar 28, 2022

Uh oh!

captainzmc commented Mar 28, 2022

Uh oh!

guohao-rosicky commented Mar 28, 2022

Uh oh!

captainzmc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Mar 28, 2022

Uh oh!

szetszwo commented Mar 28, 2022

Uh oh!

szetszwo commented Mar 28, 2022

Uh oh!

captainzmc commented Mar 28, 2022

Uh oh!

captainzmc commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Mar 28, 2022

Uh oh!

captainzmc commented Apr 19, 2022

Uh oh!

szetszwo commented Apr 19, 2022

Uh oh!

captainzmc commented Apr 20, 2022

Uh oh!

szetszwo commented Apr 20, 2022

Uh oh!

captainzmc commented Apr 21, 2022

Uh oh!

szetszwo commented Apr 21, 2022

Uh oh!

szetszwo commented May 13, 2022

Uh oh!

captainzmc commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented May 13, 2022

Uh oh!

guohao-rosicky commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

guohao-rosicky commented Mar 24, 2022 •

edited

Loading

captainzmc Mar 27, 2022 •

edited

Loading

captainzmc commented Mar 28, 2022 •

edited

Loading

captainzmc left a comment •

edited

Loading

captainzmc commented Mar 28, 2022 •

edited

Loading

captainzmc commented May 13, 2022 •

edited

Loading

guohao-rosicky commented May 13, 2022 •

edited

Loading