HDDS-5869. [Ozone-Streaming] Added support for stream on S3Gateway wr… #2858

guohao-rosicky · 2021-11-22T07:22:08Z

What changes were proposed in this pull request?

Added support for stream on S3Gateway write path

What is the link to the Apache JIRA

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

guohao-rosicky · 2021-11-22T07:22:56Z

hi @szetszwo @captainzmc , can you help review this patch?

szetszwo

@guohao-rosicky , thanks for working on this.

This is tricky since we want to have zero buffer copying. Let's study how to have zero buffer copying with JAX-RS.

szetszwo · 2021-11-22T12:21:34Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

Typo? Why hadoop.conf but not hadoop.hdds.conf?

szetszwo · 2021-11-22T12:28:04Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

We should make streaming optional. Add a new QueryParam with DefaultValue not using streaming.

+ @QueryParam("streaming") @DefaultValue("false") boolean streaming,

Use streaming only if it is true.

@szetszwo , when @QueryParam("streaming") is true, use streaming? aws s3 should not have this option.
see: https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPUT.html

when @QueryParam("streaming") is true, use streaming?

Yes.

... aws s3 should not have this option.

Similar to partNumber and uploadId, this parameter is Ozone specific. We may change the default to true later on when the streaming feature is ready.

@szetszwo I can add a configuration to s3 Gateway

@szetszwo and @guohao-rosicky I donot have much context on Streaming. But my comment is from the perspective of S3.

Similar to partNumber and uploadId, this parameter is Ozone specific. We may change the default to true later on >when the streaming feature is ready.

This is also part of AWS S3 protocol for multipart upload.

Adding query param streaming to this API and then passing query param from AWS S3 SDK will not be possible (As Ozone S3 talks S3 protocol only). I think based on the config selecting streaming or non-streaming would be the way to go here.

This is also part of AWS S3 protocol for multipart upload.

@bharatviswa504 , thanks for the info. You are right that partNumber and uploadId are also in AWS; see https://docs.aws.amazon.com/AmazonS3/latest/API/API_UploadPart.html

Adding query param streaming to this API and then passing query param from AWS S3 SDK will not be possible ...

It is still better to add a query param and set the default to false (non-streaming) for now. After streaming is stable, we will change the default to true. We need this param for testing and branch mark without restarting the machines. We should set the param name to something like "internalOzoneParam-streaming" to emphasize that it is not in the AWS S3 protocol.

AWS S3 SDK will always use the default.

We may have a conf to override the default value if such conf is useful.

szetszwo · 2021-11-22T13:40:26Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

We should avoid buffer copying. I wonder if we should other body type (instead of InputStream) in order to support zero buffer copying; see https://docs.oracle.com/cd/E19798-01/821-1841/6nmq2cp2b/index.html#gkccg

@szetszwo I looked at this document, Is it available java.io.File All media types (*/*)?

Reading files is similar to the implementation here: https://github.com/apache/ozone/blob/HDDS-4454/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/PutKeyHandler.java#L134

@szetszwo I submitted the new code, is it ok to change it like this, please take a look.

Such a change is a little big, and the previous UT needs to be changed

... Is it available java.io.File All media types (/)?

We should try it.

Such a change is a little big, and the previous UT needs to be changed

I suggest we should change the case when uploadId == null (non-createMultipartKey) first. Then, change the createMultipartKey case in a separate JIRA.

@szetszwo
like this

public Response put( @PathParam("bucket") String bucketName, @PathParam("path") String keyPath, @HeaderParam("Content-Length") long length, @QueryParam("partNumber") int partNumber, @QueryParam("uploadId") @DefaultValue("") String uploadID, File bodyFile) throws IOException, OS3Exception {

If change InputStream to File, all UT related to put cannot be used. Is there any way to solve this problem

For example here

org.apache.hadoop.ozone.s3.endpoint.TestObjectPut

@Test public void testPutObject() throws IOException, OS3Exception { //GIVEN HttpHeaders headers = Mockito.mock(HttpHeaders.class); ByteArrayInputStream body = new ByteArrayInputStream(CONTENT.getBytes(UTF_8)); objectEndpoint.setHeaders(headers); //WHEN Response response = objectEndpoint.put(bucketName, keyName, CONTENT .length(), 1, null, body); //THEN OzoneInputStream ozoneInputStream = clientStub.getObjectStore().getS3Bucket(bucketName) .readKey(keyName); String keyContent = IOUtils.toString(ozoneInputStream, UTF_8); Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(CONTENT, keyContent); }

put method Parameters for the InputStream

If streaming is enabled through configuration, do you need to retain the InputStream?

We may add another put method as below:

+++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java @@ -155,6 +153,24 @@ public Response put( @HeaderParam("Content-Length") long length, @QueryParam("partNumber") int partNumber, @QueryParam("uploadId") @DefaultValue("") String uploadID, + @QueryParam("ozoneInternalParam_streaming") @DefaultValue("false") + boolean ozoneInternalParam_streaming, + File body) throws IOException, OS3Exception { + if (!ozoneInternalParam_streaming) { + final BufferedInputStream in = new BufferedInputStream(new FileInputStream(body)); + return put(bucketName, keyPath, length, partNumber, uploadID, in); + } + + // new code for Ratis streaming + ... + } + + public Response put( + String bucketName, + String keyPath, + long length, + int partNumber, + String uploadID, InputStream body) throws IOException, OS3Exception { OzoneOutputStream output = null;

Then, the existing tests will remain working. Of course, we need to create new tests for the new code.

Okay, I'm going to implement a version, and you can take a look

szetszwo · 2021-11-29T12:49:36Z

@guohao-rosicky , the compilation failed. Please take a looks. Thanks.

guohao-rosicky · 2021-12-22T08:17:53Z

@szetszwo This PR is implemented using buffer copy, and then I'll turn on another PR to optimize for zero copy

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

guohao-rosicky · 2021-12-22T11:44:57Z

Added the enable Stream configuration on the Gateway side.

Please take a look. @szetszwo

szetszwo

@guohao-rosicky , thanks for the update. Some comments inlined.

Please add some new tests. Otherwise, we don't know if the new code is working.

szetszwo · 2021-12-23T10:05:28Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/S3GatewayConfigKeys.java

Please reuse OzoneConfigKeys.DFS_CONTAINER_RATIS_DATASTREAM_ENABLE instead of adding a new conf.

szetszwo · 2021-12-23T10:06:07Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

Let's call it "datastreamEnabled".

szetszwo · 2021-12-23T10:08:34Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

Let's move the new code to a new class, say ObjectEndpointStreaming. Otherwise, there are many changes here and it becomes hard to maintain.

@PUT public Response put( @PathParam("bucket") String bucketName, @PathParam("path") String keyPath, @HeaderParam("Content-Length") long length, @QueryParam("partNumber") int partNumber, @QueryParam("uploadId") @DefaultValue("") String uploadID, InputStream body) throws IOException, OS3Exception { if (datastreamEnabled) { return objectEndpointStreaming.put(bucketName, keyPath, length, partNumber, uploadID, body); } ... }

…treamCommitWatcher. (apache#3492)

…o put EC key. (apache#3516)

captainzmc · 2022-06-24T06:18:52Z

Thanks @guohao-rosicky update this. @szetszwo Can you help take another look?

szetszwo

@guohao-rosicky , thanks a lot for working on this. Are the crypto changes related to S3Gateway? Could we separated it out since this is very big?

Also, please revert all the whitespace changes, especially for the files existing in the master branch. Otherwise, the merge will be harder.

szetszwo · 2022-06-26T03:39:07Z

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java

Please don't have whitespace change like this. Since this is an existing file in the master branch, it will make the merge harder.

szetszwo · 2022-06-26T03:40:14Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

Please make the new fields final.

Indeed, we should create an ObjectEndpointStreaming streaming object and use streaming != null for checking if streaming is enabled.

szetszwo · 2022-06-26T03:41:13Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

Please add a local variable. Otherwise, it will affect other calls.

…ite path

guohao-rosicky · 2022-06-28T02:03:02Z

Are the crypto changes related to S3Gateway? Could we separated it out since this is very big?

Hi, @szetszwo I can work this in https://issues.apache.org/jira/browse/HDDS-5892

szetszwo · 2022-06-29T19:56:07Z

@guohao-rosicky , @captainzmc , this pull request is around for quite a long time. We should plan about how to get this committed. One problem is that this involves too much code change. Let's separate the individual methods put(..) and createMultipartKey(..) into different pull requests.

Also, we should refactor the ObjectEndpoint code in master first. Otherwise, merging becomes a pain. Let me work on the refactoring.

szetszwo

@guohao-rosicky , really appreciated that you work on this over a long time! Some comments inlined.

szetszwo · 2022-06-29T20:05:12Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

+      boolean enableEC = false;
+      if ((replicationConfig != null &&
+              replicationConfig.getReplicationType() == EC) ||
+          bucket.getReplicationConfig() instanceof ECReplicationConfig) {
+        enableEC = true;
+      }


This is incorrect when replicationConfig != null && replicationConfig.getReplicationType() != EC. As an isEc() method as below.

static boolean isEc(ReplicationConfig replication, OzoneBucket bucket) { if (replication != null) { return replication.getReplicationType() == EC; } else { return bucket.getReplicationConfig() instanceof ECReplicationConfig; } }

Hi @szetszwo , I think we'd better add a separate switch for S3G like we did for OFS/O3FS. Instead of using DFS_CONTAINER_RATIS_DATASTREAM_ENABLE to control both S3G and HDDS. This allows both client and HDDS to have a switch that controls whether or not streaming is used.
Otherwise, it cannot meet the requirements of ofs/o3fs using Streaming and S3 using Async.

Sure, a separated conf sounds good.

szetszwo · 2022-06-29T20:06:07Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java


  private List<String> customizableGetHeaders = new ArrayList<>();
  private int bufferSize;
+  private int chunkSize;


Move chunkSize to ObjectEndpointStreaming

szetszwo · 2022-06-29T20:07:18Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

Indeed, we should create an ObjectEndpointStreaming streaming object and use streaming != null for checking if streaming is enabled.

szetszwo · 2022-06-29T20:09:33Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

+      String copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER);
+      String storageType = headers.getHeaderString(STORAGE_CLASS_HEADER);
+      ReplicationConfig replicationConfig =
+          getReplicationConfig(ozoneBucket, storageType);
+
+      boolean enableEC = false;
+      if ((replicationConfig != null &&
+          replicationConfig.getReplicationType()  == EC) ||
+          ozoneBucket.getReplicationConfig() instanceof ECReplicationConfig) {
+        enableEC = true;
+      }
+
      try {
+        if (datastreamEnabled && !enableEC && copyHeader != null) {
+          Pair<String, String> result = parseSourceHeader(copyHeader);
+          String sourceBucket = result.getLeft();
+          String sourceKey = result.getRight();
+          OzoneBucket sourceOzoneBucket = getBucket(sourceBucket);
+          return ObjectEndpointStreaming
+              .copyMultipartKey(Pair.of(sourceOzoneBucket, sourceKey),
+                  Pair.of(ozoneBucket, key), length, partNumber, uploadID,
+                  chunkSize, headers);
+        } else if (datastreamEnabled && !enableEC) {
+          return ObjectEndpointStreaming
+              .createMultipartKey(ozoneBucket, key, length, partNumber,
+                  uploadID, chunkSize, body);
+        }
+


Move the logic to ObjectEndpointStreaming.

szetszwo · 2022-06-29T20:17:49Z

Filed https://issues.apache.org/jira/browse/HDDS-6973 for the refactoring.

guohao-rosicky · 2022-06-30T07:21:35Z

Hi @szetszwo #3569 has been merged into master, please rebase HDDS-4454, I will modify this PR later, thanks.

captainzmc · 2022-07-04T12:55:37Z

Hi @szetszwo #3569 has been merged into master, please rebase HDDS-4454, I will modify this PR later, thanks.

Hi @guohao-rosicky, I just had update HDDS-4454, you can continue this.

kerneltime · 2022-11-14T17:24:32Z

@szetszwo should this PR be revisited?

swamirishi · 2023-01-28T01:47:55Z

@guohao-rosicky Can you rebase this branch with master? Close this PR & open a new PR with master if you want the changes to go in. HDDS-4454 has been merged with master. Correct me if I am wrong @szetszwo @kerneltime

guohao-rosicky · 2023-01-30T11:40:55Z

@guohao-rosicky Can you rebase this branch with master? Close this PR & open a new PR with master if you want the changes to go in. HDDS-4454 has been merged with master. Correct me if I am wrong @szetszwo @kerneltime

sure.

szetszwo reviewed Nov 22, 2021

View reviewed changes

szetszwo force-pushed the HDDS-4454 branch from c117885 to 320ae54 Compare November 30, 2021 09:10

szetszwo force-pushed the HDDS-4454 branch from 99ddf5a to d1e4eea Compare December 15, 2021 13:17

captainzmc force-pushed the HDDS-4454 branch from d1e4eea to 89a12ad Compare December 20, 2021 06:26

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch 3 times, most recently from 3523cd6 to 9d007cd Compare December 22, 2021 10:45

szetszwo reviewed Dec 23, 2021

View reviewed changes

szetszwo force-pushed the HDDS-4454 branch from 7ce4e01 to ab016f3 Compare December 30, 2021 07:59

szetszwo force-pushed the HDDS-4454 branch from 1ceae85 to e8af582 Compare January 20, 2022 09:14

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch 2 times, most recently from fecd0f4 to 9f781ef Compare January 24, 2022 06:20

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from 1b3af21 to 49bd7ba Compare February 8, 2022 02:23

captainzmc force-pushed the HDDS-4454 branch from e8af582 to dba70bb Compare February 9, 2022 11:14

captainzmc mentioned this pull request Feb 10, 2022

HDDS-6282. Fix BlockDataStreamOutput#doFlushIfNeeded NPE #3060

Merged

szetszwo force-pushed the HDDS-4454 branch 2 times, most recently from f637b1e to ce1d880 Compare February 16, 2022 14:47

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch 2 times, most recently from 3bb41d4 to f6dd95f Compare February 22, 2022 08:48

szetszwo force-pushed the HDDS-4454 branch from 5395f71 to b6f1921 Compare March 15, 2022 06:55

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from f6dd95f to 6f307e3 Compare March 15, 2022 08:35

szetszwo force-pushed the HDDS-4454 branch from b6f1921 to f826d12 Compare March 16, 2022 17:30

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from d61543a to 7ef985f Compare March 18, 2022 11:12

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

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from 7ef985f to bff6e56 Compare March 29, 2022 09:47

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from d8ddda7 to 2f9cbb3 Compare April 10, 2022 01:04

szetszwo force-pushed the HDDS-4454 branch from 185a28c to 19b8f37 Compare June 9, 2022 03:05

szetszwo and others added 2 commits June 9, 2022 23:40

HDDS-6842. [Ozone-Streaming] Reduce the number of watch requests in S…

cabc517

…treamCommitWatcher. (apache#3492)

HDDS-6867. [Ozone-Streaming] PutKeyHandler should not use streaming t…

a4199f5

…o put EC key. (apache#3516)

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from cd2d180 to 9f86764 Compare June 24, 2022 04:52

guohao-rosicky requested review from captainzmc and szetszwo June 24, 2022 08:18

szetszwo requested changes Jun 26, 2022

View reviewed changes

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from 9f86764 to 473c302 Compare June 27, 2022 04:32

HDDS-5869. [Ozone-Streaming] Added support for stream on S3Gateway wr…

a6f5c40

…ite path

guohao-rosicky force-pushed the HDDS-4454-gateway-stream branch from 473c302 to a6f5c40 Compare June 27, 2022 06:12

guohao-rosicky added 2 commits June 27, 2022 19:37

trigger new CI

98d6c07

trigger new CI

0ff6a76

guohao-rosicky requested a review from szetszwo June 28, 2022 02:03

szetszwo reviewed Jun 29, 2022

View reviewed changes

guohao-rosicky mentioned this pull request Jun 30, 2022

HDDS-6973. Refactor ObjectEndpoint. #3569

Merged

captainzmc force-pushed the HDDS-4454 branch from a4199f5 to 2227d33 Compare July 4, 2022 12:53

szetszwo force-pushed the HDDS-4454 branch from 4c7151b to c84da64 Compare September 14, 2022 13:35

szetszwo force-pushed the HDDS-4454 branch from c84da64 to 653a120 Compare October 25, 2022 01:15

szetszwo force-pushed the HDDS-4454 branch from eedd983 to cfdf9cd Compare November 7, 2022 18:11

szetszwo force-pushed the HDDS-4454 branch from c17be70 to 226454d Compare December 1, 2022 03:15

guohao-rosicky closed this Jan 30, 2023

HDDS-5869. [Ozone-Streaming] Added support for stream on S3Gateway wr… #2858

HDDS-5869. [Ozone-Streaming] Added support for stream on S3Gateway wr… #2858

Uh oh!

Conversation

guohao-rosicky commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

Uh oh!

guohao-rosicky commented Nov 22, 2021

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 Nov 23, 2021 • 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!

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!

guohao-rosicky Nov 23, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

guohao-rosicky Nov 24, 2021 • 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!

guohao-rosicky Nov 25, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Nov 25, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Nov 29, 2021

Uh oh!

guohao-rosicky commented Dec 22, 2021

Uh oh!

guohao-rosicky commented Dec 22, 2021

Uh oh!

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

captainzmc commented Jun 24, 2022

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guohao-rosicky commented Nov 22, 2021 •

edited

Loading

bharatviswa504 Nov 23, 2021 •

edited

Loading

guohao-rosicky Nov 23, 2021 •

edited

Loading

guohao-rosicky Nov 24, 2021 •

edited

Loading

guohao-rosicky Nov 25, 2021 •

edited

Loading

szetszwo Nov 25, 2021 •

edited

Loading

swamirishi commented Jan 28, 2023 •

edited

Loading