Implement Netty based s3 API in worker #17661

Jackson-Wang-7 · 2023-06-21T10:29:20Z

What changes are proposed in this pull request?

Implement Netty based s3 API in worker

Why are the changes needed?

Please clarify why the changes are needed. For instance,

If you propose a new API, clarify the use case for a new API.
If you fix a bug, describe the bug.

Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including

change in user-facing APIs
addition or removal of property keys
webui

…_api

Jackson-Wang-7 · 2023-06-25T08:44:52Z

@lucyge2022 @JiamingMai @beinan please take a review about this PR. I started the pipeline of the HTTP protocol on the existing NettyDataServer to process S3 requests. Currently, it supports headObject and getObject (including zero-copy and non-zero-copy). I'm working on the listObject, putObject and other Ops.

…_api

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3HttpHandler.java

apc999

Thanks for the great work! left some comments mostly in style and convention

dora/core/server/common/pom.xml

dora/core/server/common/src/main/java/alluxio/s3/NettyRestUtils.java

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3HttpHandler.java

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3HttpPipelineHandler.java

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyBaseTask.java

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3HttpPipelineHandler.java

…_api � Conflicts: � dora/tests/src/test/java/alluxio/client/rest/CreateBucketTest.java

lucyge2022 · 2023-07-22T00:22:23Z

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyObjectTask.java

+                pagedFileReader.getMultipleDataFileChannel(mHandler.getContext().channel(), length);
+          }
+          if (packet != null) {
+            mHandler.processTransferResponse(packet);


if we read from pagestore, which is a list of defaultfileregion opened for multiple pages for the total lengthed object, where the buffer is read thru the page files when u process them one by one, will it race with other reads thru Netty data reader? Meaning will we be reading a page where it no long exist any more during the read?

Don't worry, that's what we do in the rpc reading. I follow the exact same page reading process here and I have confirmed the opinions of Jiaming and Bowen on this process。 it's okay.

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyObjectTask.java

lucyge2022 · 2023-07-22T17:04:14Z

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyHandler.java

+    ByteBuf buf = mContext.channel().alloc().buffer(packetSize, packetSize);
+    try {
+      while (buf.writableBytes() > 0 && blockReader.transferTo(buf) != -1) {
+        mContext.write(new DefaultHttpContent(buf));


why not directly write buf?

We need to pass it package by package here, so we wrapped it with HTTPContent.

lucyge2022 · 2023-07-22T17:06:50Z

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyHandler.java

+    try {
+      while (buf.writableBytes() > 0 && blockReader.transferTo(buf) != -1) {
+        mContext.write(new DefaultHttpContent(buf));
+        buf = mContext.channel().alloc().buffer(packetSize, packetSize);


can we reuse a buf here instead of alloc everytime?
also is processMappedResponse ever going to be used now that all BlockReaders are actually PagedFileReader?

I tried to reuse the buf before but I can't control when the channel release the buffer after I write it to the channel. So I just alloc it for the channel and then let channel release them after used.
this processMappedResponse is for the transfer type of MAPPED. This is indeed a limitation of FileRegion, which uses zero-copy and avoids putting data in the user space. But TLS needs to rewrite the data on the fly before sending it, so these two are incompatible

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyObjectTask.java

lucyge2022 · 2023-07-22T17:57:41Z

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyObjectTask.java

+        // the encoding type.
+        boolean isChunkedEncoding = decodedLengthHeader != null;
+        long toRead;
+        ByteBuf buf = mHandler.getRequestContent();


so FullHttpRequest contains the entire http body? The HttpObjectAggregator helps to aggregate entire http body into a fullhttprequest and then invoke channelread ? Have u tried upload a obj > 512KB ? And what if upload a 5G obj? is this buf gonna be of 5G in size?

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3HttpHandler.java

Jackson-Wang-7 · 2023-07-25T06:42:00Z

@apc999 @lucyge2022 Based on the review comments, I have modified the relevant code. Can you take a look again and see if there are any other code lines that need to be modified?

Currently, only large file uploads are not supported, but considering that this may be a big and significant modification, can I support large file uploads in the next PR?

…by default

Jackson-Wang-7 · 2023-07-28T06:05:41Z

@apc999 @lucyge2022 Any other comments or changement suggestion for this PR? It is a basic PR, some functions are not currently supported. I will continue to supplement and improve them in subsequent PRs.

…_api � Conflicts: � dora/minicluster/src/main/java/alluxio/multi/process/MultiProcessCluster.java � dora/tests/src/test/java/alluxio/client/rest/RestApiTest.java

lucyge2022

LGTM

apc999

thanks ! it looks good mostly, leaving another batch of comments.

dora/core/common/src/main/java/alluxio/util/network/NetworkAddressUtils.java

dora/core/server/common/pom.xml

dora/core/server/common/src/main/java/alluxio/RestUtils.java

dora/core/server/common/src/main/java/alluxio/s3/NettyRestUtils.java

apc999 · 2023-08-01T06:54:57Z

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyHandler.java

+          requestHeaders, responseHeaders);
+      LOG.debug(accessLog + " " + moreInfoStr);
+    } else {
+      LOG.info(accessLog);


do we log every access?

yes, we log some basic info for every access request.

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3NettyHandler.java

…_api

apc999

Minor comments left. LGTM!

thanks for the contribution

dora/core/common/src/main/java/alluxio/util/network/NetworkAddressUtils.java

apc999 · 2023-08-03T05:51:12Z

dora/core/server/common/src/main/java/alluxio/s3/S3ErrorResponse.java

+    } else if (e instanceof IOException) {
+      return createNettyErrorResponse((IOException) e, resource);
+    } else {
+      ByteBuf contentBuffer =


can we put the allocation of contentBuff from Unpooled.copiedBuffer also into generateS3ErrorResponse, so generateS3ErrorResponse takes a message String as arg in stead of ByteBuff?

ex.

else { return generateS3ErrorResponse(HttpResponseStatus.INTERNAL_SERVER_ERROR, e.getMessage(), HttpHeaderValues.TEXT_PLAIN); }

the reason is netty buffer allocation can be very tricky, so we want to reduce the locations where we allocate Netty ByteBuf (like using Unpooled.copiedBuffer). Typically, Netty ByteBuf requires calling release to reclaim the resource, but Unpooled.copiedBuffer is an exception. So let's try to consolidate these allocations and it is easier to manage in the future.

good catch, I will change it

Jackson-Wang-7 · 2023-08-03T06:56:29Z

alluxio-bot, merge this please

Jackson-Wang-7 added 4 commits June 20, 2023 14:20

support headObject in netty server

ef8afa8

getObject API demo (can work)

83d77be

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

2a32752

…_api

refactoring some method and fix checkstyle

2a656d8

Jackson-Wang-7 changed the title ~~Implement Netty based s3 API in worker(WIP)~~ Implement Netty based s3 API in worker Jun 25, 2023

Jackson-Wang-7 requested review from JiamingMai, beinan and lucyge2022 June 25, 2023 08:40

Jackson-Wang-7 added 21 commits June 25, 2023 18:48

support listAllBucket

ef0bc35

fix checkstyle

12fba3b

support listObject

36889a5

fix checkstyle

eba6140

support redirect in netty s3(WIP)

17d33ae

fix some bugs of the Chunked read in GetObject

452ff4d

fix some bugs of the Chunked read in GetObject

a745fbc

support putObject and headBucket

a8eead8

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

c4f08ed

…_api

fix some bugs of getObjects

addfafe

add access log for netty s3 api

8e4994d

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

4af6f2b

…_api

get content hash from FileInfo

9138fc2

fix checkstyle

8b373a8

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

1d558b0

…_api

enable AuditLogWriter

22ef2a3

fix unit test

b198d16

fix unit test

63f25af

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

95ee41f

…_api

support chunked encoding in create object

bcc769b

change the way to get etag of file

83d383d

Jackson-Wang-7 added 4 commits July 17, 2023 09:52

support heavyPool and lightPool

4e8a268

fix checkstyle

3d58e7f

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

ef45c9a

…_api

pull the latest main

36b691d

lucyge2022 reviewed Jul 17, 2023

View reviewed changes

dora/core/server/worker/src/main/java/alluxio/worker/s3/S3HttpHandler.java Outdated Show resolved Hide resolved

apc999 requested changes Jul 21, 2023

View reviewed changes

Jackson-Wang-7 added 5 commits July 24, 2023 17:11

fix review comments

6f58767

fix checkstyle

b96e107

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

909dc33

…_api � Conflicts: � dora/tests/src/test/java/alluxio/client/rest/CreateBucketTest.java

fix checkstyle

f036961

fix checkstyle

7eee172

lucyge2022 reviewed Jul 25, 2023

View reviewed changes

Jackson-Wang-7 added 2 commits July 25, 2023 14:17

fix async task issue

5dee107

fix async task issue

7e87373

Don't support putObject && Add the switch to disable netty s3 server …

46b0420

…by default

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

9eda306

…_api � Conflicts: � dora/minicluster/src/main/java/alluxio/multi/process/MultiProcessCluster.java � dora/tests/src/test/java/alluxio/client/rest/RestApiTest.java

lucyge2022 approved these changes Jul 31, 2023

View reviewed changes

fix merge conflict

6dab9b3

apc999 requested changes Aug 1, 2023

View reviewed changes

Jackson-Wang-7 added 5 commits August 1, 2023 16:50

fix some review comments

7787793

fix checkstyle

a204f4b

fix checkstyle

acd07f6

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

3bb1baf

…_api

Merge branch 'main' of github.com:Alluxio/alluxio into netty_based_s3…

a0ccd41

…_api

apc999 approved these changes Aug 3, 2023

View reviewed changes

fix some review comments

c019b3d

alluxio-bot merged commit dbb6af2 into Alluxio:main Aug 3, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Netty based s3 API in worker #17661

Implement Netty based s3 API in worker #17661

Jackson-Wang-7 commented Jun 21, 2023

Jackson-Wang-7 commented Jun 25, 2023

apc999 left a comment

lucyge2022 Jul 22, 2023

Jackson-Wang-7 Jul 25, 2023

lucyge2022 Jul 22, 2023

Jackson-Wang-7 Jul 25, 2023

lucyge2022 Jul 22, 2023

Jackson-Wang-7 Jul 25, 2023

lucyge2022 Jul 22, 2023

Jackson-Wang-7 commented Jul 25, 2023

Jackson-Wang-7 commented Jul 28, 2023

lucyge2022 left a comment

apc999 left a comment

apc999 Aug 1, 2023

Jackson-Wang-7 Aug 1, 2023

apc999 left a comment

apc999 Aug 3, 2023

Jackson-Wang-7 Aug 3, 2023

Jackson-Wang-7 commented Aug 3, 2023

Implement Netty based s3 API in worker #17661

Implement Netty based s3 API in worker #17661

Conversation

Jackson-Wang-7 commented Jun 21, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

Jackson-Wang-7 commented Jun 25, 2023

apc999 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackson-Wang-7 commented Jul 25, 2023

Jackson-Wang-7 commented Jul 28, 2023

lucyge2022 left a comment

Choose a reason for hiding this comment

apc999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apc999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackson-Wang-7 commented Aug 3, 2023