Add builder support to Compose object api #979

sinhaashish · 2020-06-11T21:15:54Z

Changes with this PR

ComposeSource class changed , the copyCondition is removed.
ComposeSource class extends ObjectVersionArgs and builder pattern is used to initialise it.
ComposeObject uses copyObject in case partsCount is 1.
uploadPartCopy and createMultipartUpload started using MultiMap.
Returns ObjectWriteResponse for copyObject and composeObject

api/src/main/java/io/minio/BaseArgs.java

api/src/main/java/io/minio/ComposeObjectArgs.java

api/src/main/java/io/minio/ComposeSourceArgs.java

anjalshireesh · 2020-06-12T10:59:25Z

api/src/main/java/io/minio/ComposeSourceArgs.java

+    @Override
+    protected void validate(ComposeSourceArgs args) {
+      super.validate(args);
+      validateBucketName(args.bucket());


This is not required as super.validate(args) internally invokes validateBucketName anyway

api/src/main/java/io/minio/MinioClient.java

functional/FunctionalTest.java

anjalshireesh

LGTM, apart for one small comment. Also, you need to rebase and resolve conflicts.

anjalshireesh · 2020-06-16T14:14:32Z

api/src/main/java/io/minio/ComposeSource.java

+  /** Argument builder of {@link ComposeSource}. */
+  public static final class Builder extends ObjectVersionArgs.Builder<Builder, ComposeSource> {
+    @Override
+    protected void validate(ComposeSource args) {


Since this method is not doing anything apart from calling the super method, it is not required anymore.

anjalshireesh

LGTM

docs/API.md

api/src/main/java/io/minio/ComposeObjectArgs.java

api/src/main/java/io/minio/ComposeSource.java

api/src/main/java/io/minio/MinioClient.java

functional/FunctionalTest.java

balamurugana · 2020-06-18T02:57:44Z

functional/FunctionalTest.java

+  private static void deleteFilesAndObjects(String bucketName, String files[]) throws Exception {
+    for (String filename : files) {
+      Files.delete(Paths.get(filename));
+      client.removeObject(RemoveObjectArgs.builder().bucket(bucketName).object(filename).build());


This method is not necessary. you could use removeObjects() then this for

This method is used to make the functional Test less verbose as the same code was repeating 6 composeObject test . I have used removeObjects now.

api/src/main/java/io/minio/ComposeObjectArgs.java

docs/API.md

api/src/main/java/io/minio/MinioClient.java

functional/FunctionalTest.java

api/src/main/java/io/minio/MinioClient.java

balamurugana · 2020-06-21T03:34:48Z

api/src/main/java/io/minio/MinioClient.java

      src.buildHeaders(stat.length(), stat.etag());

-      if (i != 0 && src.headers().containsKey("x-amz-meta-x-amz-key")) {
+      if (i != 0 && src.extraHeaders().containsKey("x-amz-meta-x-amz-key")) {


You would need to use headers() not extraHeaders(). headers member data is updated in ComposeSource.buildHeaders()

api/src/main/java/io/minio/MinioClient.java

docs/API.md

balamurugana · 2020-06-22T03:09:42Z

api/src/main/java/io/minio/MinioClient.java

@@ -2647,49 +2715,45 @@ public void composeObject(
      }
    }

-    if (partsCount == 1) {
+    if (args.sources().size() == 1) {


You would need to do if (args.sources.size() == 1 && src.offset() == null && src.length() == null) {

No need to use sources.get(0) because src is already pointing to sources.get(0)

balamurugana · 2020-06-22T03:13:03Z

api/src/main/java/io/minio/MinioClient.java

-            "bytes=" + src.offset() + "-" + (src.offset() + src.length() - 1));
+      Multimap<String, String> headers = HashMultimap.create();
+      headers.putAll(args.extraHeaders());
+      headers.putAll(args.headers);


Isn't it headers.putAll(args.headers()) ? Not sure why there is no compiler error to this line.

Why is it required to create combined headers here? Can't you use them directly like .extraHeaders(args.extraHeaders()).headers(args.headers()) ?

balamurugana · 2020-06-22T03:27:13Z

api/src/main/java/io/minio/MinioClient.java

    }

-    String uploadId = createMultipartUpload(bucketName, objectName, headerMap);
+    String uploadId = createMultipartUpload(args.bucket(), null, args.object(), headerMap, null);


use createMultipartUpload() like String uploadId = createMultipartUpload(args.bucket(), args.region(), args.object(), args.genHeaders(), args.extraQueryParams());

move this line at 1242

balamurugana · 2020-06-22T03:32:23Z

api/src/main/java/io/minio/MinioClient.java

+    if (args.sse() != null) {
+      sseHeaders.putAll(Multimaps.forMap(args.sse().headers()));
+      headerMap.putAll(args.extraHeaders());
+      headerMap.putAll(args.headers);


Isn't it headers.putAll(args.headers()) ? Not sure why there is no compiler error to this line.

balamurugana · 2020-06-22T03:34:55Z

api/src/main/java/io/minio/MinioClient.java

      throw e;
    } catch (Exception e) {
-      abortMultipartUpload(bucketName, objectName, uploadId);
+      abortMultipartUpload(args.bucket(), args.object(), uploadId);


abortMultipartUpload() should be called not if (args.sources.size() == 1 && src.offset() == null && src.length() == null) {

balamurugana · 2020-06-22T03:35:06Z

api/src/main/java/io/minio/MinioClient.java

    } catch (RuntimeException e) {
-      abortMultipartUpload(bucketName, objectName, uploadId);
+      abortMultipartUpload(args.bucket(), args.object(), uploadId);


abortMultipartUpload() should be called not if (args.sources.size() == 1 && src.offset() == null && src.length() == null) {

kannappanr

LGTM

sinhaashish requested review from balamurugana and anjalshireesh June 11, 2020 21:15

anjalshireesh requested changes Jun 12, 2020

View reviewed changes

sinhaashish force-pushed the add-builder-compose-object branch 2 times, most recently from 0cf5205 to 8c65613 Compare June 15, 2020 09:54

anjalshireesh previously approved these changes Jun 16, 2020

View reviewed changes

sinhaashish dismissed anjalshireesh’s stale review via 3b5d42f June 17, 2020 09:45

sinhaashish force-pushed the add-builder-compose-object branch 3 times, most recently from 2e013da to 31168c1 Compare June 17, 2020 13:20

anjalshireesh previously approved these changes Jun 17, 2020

View reviewed changes

balamurugana reviewed Jun 18, 2020

View reviewed changes

sinhaashish dismissed anjalshireesh’s stale review via 99b7354 June 18, 2020 15:43

balamurugana reviewed Jun 19, 2020

View reviewed changes

sinhaashish added 6 commits June 20, 2020 14:35

Add builder support to Compose object api

53866d1

Address review comments

d105add

ObjectWriteResponse from composeObject & copyObject

099e360

Modify ObjectWriteResponse

cfe2334

Address Bala's review comment

1b5f96f

Address more comments

21d5df8

sinhaashish force-pushed the add-builder-compose-object branch from 788562e to 21d5df8 Compare June 20, 2020 17:36

balamurugana reviewed Jun 21, 2020

View reviewed changes

sinhaashish force-pushed the add-builder-compose-object branch from e22aef8 to 0a93db6 Compare June 22, 2020 02:33

resolving comments

40934b0

sinhaashish force-pushed the add-builder-compose-object branch from 0a93db6 to 40934b0 Compare June 22, 2020 02:40

balamurugana reviewed Jun 22, 2020

View reviewed changes

Handle sse

105b906

sinhaashish force-pushed the add-builder-compose-object branch from 65557a6 to 105b906 Compare June 22, 2020 09:47

balamurugana approved these changes Jun 22, 2020

View reviewed changes

kannappanr approved these changes Jun 22, 2020

View reviewed changes

balamurugana merged commit 0014bf3 into minio:master Jun 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add builder support to Compose object api #979

Add builder support to Compose object api #979

sinhaashish commented Jun 11, 2020 •

edited

Loading

anjalshireesh Jun 12, 2020

anjalshireesh left a comment

anjalshireesh Jun 16, 2020

anjalshireesh left a comment

balamurugana Jun 18, 2020

sinhaashish Jun 18, 2020 •

edited

Loading

balamurugana Jun 21, 2020

balamurugana Jun 22, 2020

balamurugana Jun 22, 2020

balamurugana Jun 22, 2020

balamurugana Jun 22, 2020

balamurugana Jun 22, 2020

balamurugana Jun 22, 2020

kannappanr left a comment

Add builder support to Compose object api #979

Add builder support to Compose object api #979

Conversation

sinhaashish commented Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

anjalshireesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjalshireesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhaashish Jun 18, 2020 • edited Loading

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

kannappanr left a comment

Choose a reason for hiding this comment

sinhaashish commented Jun 11, 2020 •

edited

Loading

sinhaashish Jun 18, 2020 •

edited

Loading