POC for S3 API experiments (Not to be checked in) #2664

Arun-LinkedIn · 2023-12-01T21:28:50Z

No description provided.

codecov-commenter · 2023-12-01T21:37:23Z

Codecov Report

Attention: 554 lines in your changes are missing coverage. Please review.

Comparison is base (6edfb75) 72.63% compared to head (399af24) 39.76%.
Report is 23 commits behind head on master.

Files	Patch %	Lines
...om/github/ambry/frontend/NamedBlobPostHandler.java	0.00%	228 Missing ⚠️
...ava/com/github/ambry/frontend/HeadBlobHandler.java	0.00%	46 Missing ⚠️
...om/github/ambry/frontend/NamedBlobListHandler.java	0.00%	44 Missing ⚠️
...a/com/github/ambry/router/OperationController.java	2.38%	41 Missing ⚠️
...com/github/ambry/frontend/NamedBlobPutHandler.java	0.00%	35 Missing ⚠️
...hub/ambry/frontend/FrontendRestRequestService.java	0.00%	25 Missing ⚠️
...va/com/github/ambry/frontend/ListBucketResult.java	0.00%	25 Missing ⚠️
...c/main/java/com/github/ambry/rest/RequestPath.java	0.00%	20 Missing ⚠️
.../main/java/com/github/ambry/frontend/Contents.java	0.00%	16 Missing ⚠️
.../ambry/frontend/CompleteMultipartUploadResult.java	0.00%	13 Missing ⚠️
... and 10 more

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #2664       +/-   ##
=============================================
- Coverage     72.63%   39.76%   -32.88%     
+ Complexity    11537     5956     -5581     
=============================================
  Files           809      819       +10     
  Lines         65545    66408      +863     
  Branches       7997     8086       +89     
=============================================
- Hits          47611    26404    -21207     
- Misses        15334    37683    +22349     
+ Partials       2600     2321      -279

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This handles: 1. PUT, GET, DELETE, HEAD, LIST 2. Multipart uploads 3. Also made changes in frontend.properties file to start frontend on ssl port and talk to mysql

JingQianCloud · 2024-01-09T04:54:37Z

I'm thinking of adding a enum of AmazonS3_Action
https://docs.aws.amazon.com/AmazonS3/latest/API/API_Operations_Amazon_Simple_Storage_Service.html

For example, followings are related to multiple part upload.

CreateMultipartUpload
UploadPart
CompleteMultipartUpload
AbortMultipartUpload
ListParts
ListMultipartUploads

At the early stage like somewhere at preProcessAndRouteRequest
We generate the action type. In our handling code, we don't parse or rely on keywords like "upload", "uploadId" etc.

And we also generate the related parameter per each type. For example, class S3CreateMultipleUploadContext.
RestRequest has a field called RestRequestContext. We can set the command context to that variable.

JingQianCloud · 2024-01-08T20:34:36Z

ambry-api/src/main/java/com/github/ambry/frontend/NamedBlobPath.java

+    // S3 can issue "HEAD /s3/named-blob-sandbox" on the bucket-name.
+    // The converted named blob would be /named/named-blob-sandbox/container-a. So, don't check for number of expected
+    // segments for S3 as of now
+    /*


What's the case we hit the exception?
After the naming translation from S3 to "named",

For Get, we do have four parts, "named", account, container and key.

For list, we have three parts, "named", account, container. At least for flink case, looks like it's still true. If we support list under any key, this is another case.

One small issue I saw was that for listing, the path is "/named/named-blob-sandbox/container-a/". With the ending "/", we split it to four parts. But if we trim the ending "/", it works fine.

JingQianCloud · 2024-01-09T22:27:16Z

ambry-frontend/src/main/java/com/github/ambry/frontend/InitiateMultipartUploadResult.java

+import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;
+
+
+public class InitiateMultipartUploadResult {


Probably we can follow S3 naming:
CreateMultipartUploadResponse

JingQianCloud · 2024-01-10T00:18:39Z

ambry-api/src/main/java/com/github/ambry/rest/RequestPath.java

+    // We convert it to named blob request in the form  "/named/named-blob-sandbox/container-a/checkpoints/87833badf879a3fc7bf151adfe928eac/chk-1/_metadata"
+    // i.e. we hardcode container name to 'container-a'
+
+    logger.info("S3 API | Input path: {}", path);


As another comment, probably we can have one function to parse all S3 command and generate context for each S3 commands.

JingQianCloud · 2024-01-10T03:18:06Z

ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java

@@ -541,7 +556,7 @@ public static BlobProperties buildBlobProperties(Map<String, Object> args) throw
   * @throws RestServiceException if required arguments aren't present or if they aren't in the format expected.
   */
  public static long getTtlFromRequestHeader(Map<String, Object> args) throws RestServiceException {
-    long ttl = Utils.Infinite_Time;
+    long ttl = 86400;


Did we ever discuss the life cycle management with flink and TiKV team?

JingQianCloud · 2024-01-10T03:47:32Z

We ever discussed for S3, do we need dedicated handler for it?
I'm thinking inherit namedBlob handler and create s3 handler.
In S3 handler, we only overwrite the code which we need change.

Arun-LinkedIn requested a review from JingQianCloud December 1, 2023 21:42

Changes for handling S3 requests

399af24

This handles: 1. PUT, GET, DELETE, HEAD, LIST 2. Multipart uploads 3. Also made changes in frontend.properties file to start frontend on ssl port and talk to mysql

Arun-LinkedIn force-pushed the S3ExperimentChanges branch from f8a59fc to 399af24 Compare December 21, 2023 22:18

JingQianCloud reviewed Jan 10, 2024

View reviewed changes

Arun-LinkedIn changed the title ~~Changes for S3 API experiments (Not to be checked in)~~ POC for S3 API experiments (Not to be checked in) Jan 23, 2024

Add S3ListHandler

0a0b871

Arun-LinkedIn requested a review from weisong44 January 25, 2024 21:34

Use UUID as multipart upload id instead of signed url

e7a7eb2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC for S3 API experiments (Not to be checked in) #2664

POC for S3 API experiments (Not to be checked in) #2664

Arun-LinkedIn commented Dec 1, 2023

codecov-commenter commented Dec 1, 2023 •

edited

JingQianCloud commented Jan 9, 2024 •

edited

JingQianCloud Jan 8, 2024

JingQianCloud Jan 9, 2024

JingQianCloud Jan 10, 2024

JingQianCloud Jan 10, 2024

JingQianCloud commented Jan 10, 2024 •

edited

		import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;


		public class InitiateMultipartUploadResult {

POC for S3 API experiments (Not to be checked in) #2664

Are you sure you want to change the base?

POC for S3 API experiments (Not to be checked in) #2664

Conversation

Arun-LinkedIn commented Dec 1, 2023

codecov-commenter commented Dec 1, 2023 • edited

Codecov Report

JingQianCloud commented Jan 9, 2024 • edited

JingQianCloud Jan 8, 2024

Choose a reason for hiding this comment

JingQianCloud Jan 9, 2024

Choose a reason for hiding this comment

JingQianCloud Jan 10, 2024

Choose a reason for hiding this comment

JingQianCloud Jan 10, 2024

Choose a reason for hiding this comment

JingQianCloud commented Jan 10, 2024 • edited

codecov-commenter commented Dec 1, 2023 •

edited

JingQianCloud commented Jan 9, 2024 •

edited

JingQianCloud commented Jan 10, 2024 •

edited