Make bucket with builder #927

sinhaashish · 2020-05-11T06:53:35Z

No description provided.

anjalshireesh

Recommended syntax for building the make-bucket args:

MakeBucketArgs.builder()
        .bucket("<bucketname>")
        .region("<region>")
        .objectLock(<true/false>)
        .build()

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

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

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

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

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

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

balamurugana · 2020-05-11T13:46:58Z

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

+  private String region;
+
+  BucketArgs(Builder<?> builder) {
+


You can remove this whitespace.

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

balamurugana · 2020-05-11T13:50:13Z

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

+    public String name;
+    public String region;
+
+    @SuppressWarnings("unchecked")


Add justification as Its safe to type cast to T as T is inherited this class.

balamurugana · 2020-05-11T13:55:03Z

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

+    this(new Builder());
+  }
+
+  public Builder builder() {


Its been decided to add this feature later on demand.

You can find @anjalshireesh comment above, and thus i have made this as static and created the object like

MakeBucketArgs.builder() .bucket("<bucketname>") .region("<region>") .objectLock(<true/false>) .build()

balamurugana · 2020-05-11T13:56:08Z

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

@@ -1957,27 +1957,27 @@ public void getObject(
   * Creates an object by server-side copying data from another object.
   *
   * <pre>Example:{@code
-   * // Copy data from my-source-bucketname/my-objectname to my-bucketname/my-objectname.
+   *  Copy data from my-source-bucketname/my-objectname to my-bucketname/my-objectname.


This is code block. It should be a comment. Also touch relevant code to this PR.

balamurugana · 2020-05-11T14:03:01Z

docs/API.md


 __Example__
 ```java
-minioClient.makeBucket("my-bucketname", "us-west-2", true);
+// Create bucket with default region.
+minioClient.makeBucket(new MakeBucketArgs.Builder().bucket("my-bucketname").build());


Its better to have new line after each example.

balamurugana · 2020-05-11T14:03:35Z

docs/API.md

+minioClient.makeBucket(new MakeBucketArgs.Builder().bucket("my-bucketname").build());
+// Create bucket with specific region.
+minioClient.makeBucket(
+    new MakeBucketArgs.Builder().bucket("my-bucketname").region("us-west-1").build());


Make same indention like below example.

balamurugana · 2020-05-11T14:04:12Z

docs/API.md

-| ``objectLock`` | _boolean_ | Flag to enable object lock feature.                    |
+| Parameter      | Type             | Description                |
+|:---------------|:-----------------|:---------------------------|
+| ``args``       | _[MakeBucketArgs]_ | Arguments to create bucket |


You would need to add link to [MakeBucketArgs]

i have tried the below but the link is visible. Is this correct approach

__Parameters__ | Parameter | Type | Description | |:---------------|:-------------------|:---------------------------| | ``args`` | _[MakeBucketArgs](#https://minio.github.io/minio-java/io/minio/MakeBucketArgs.html)_ | Arguments to create bucket |

balamurugana · 2020-05-11T14:06:06Z

examples/MakeBucketWithLock.java

@@ -36,7 +37,8 @@ public static void main(String[] args)
        System.out.println("my-bucketname already exists");
      } else {
        // Create bucket 'my-bucketname' with object lock functionality enabled
-        s3Client.makeBucket("my-bucketname", null, true);
+        s3Client.makeBucket(
+            new MakeBucketArgs.Builder().bucket("my-bucketname").objectLock(true).build());


Now you could have one example MakeBucket.java and add three variation there.

balamurugana · 2020-05-11T14:20:14Z

functional/FunctionalTest.java

  public static void makeBucketwithRegion_test() throws Exception {
    if (!mintEnv) {
-      System.out.println("Test: makeBucket(String bucketName, String region)");
+      System.out.println("Test: makeBucket(MakeBucketArgs args)");


You can add a tag like with region, with objectLock or with region and objectLock

balamurugana · 2020-05-12T04:18:31Z

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

@@ -16,30 +16,55 @@

 package io.minio;

+/** Bucket Arguments to hold base bucket properties */


/** Base argument class holds bucket name and region */

balamurugana · 2020-05-12T04:22:32Z

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

+    this.region = builder.region;
+  }
+
+  /** Returns the name of bucket */


/** Returns bucket name */ to consistent with other docs.

balamurugana · 2020-05-12T04:22:51Z

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

+    return name;
+  }
+
+  /** Returns the region of bucket */


/** Returns region */

balamurugana · 2020-05-12T04:23:55Z

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

+    }
+
+    @SuppressWarnings("unchecked")
+    /** Its safe to type cast to T as T is inherited this class. */


Please check how to add justification in @suppressWarnings

balamurugana · 2020-05-12T04:24:33Z

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

+    }
+
+    /** Validate the name of the bucket */
+    public void validateName(String name) {


We could make this as static method.

balamurugana · 2020-05-12T04:34:47Z

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

+
+    public Builder() {}
+
+    public Builder(MakeBucketArgs args) {


You could remove Builder(XXXArgs args) constructor everywhere and will be added on demand.

balamurugana · 2020-05-12T04:42:07Z

docs/API.md

-| ``region``     | _String_ | Region in which the bucket will be created. |
+| Parameter      | Type               | Description                |
+|:---------------|:-------------------|:---------------------------|
+| ``args``       | _[MakeBucketArgs](#https://minio.github.io/minio-java/io/minio/MakeBucketArgs.html)_ | Arguments to create bucket |


Check how [Method] link is added as reference.

balamurugana · 2020-05-12T04:47:52Z

examples/MakeBucket.java

+                .region("us-west-1")
+                .objectLock(true)
+                .build());
+        System.out.println("my-bucketname3 is created successfully");


You could have bucket names as my-bucketname, my-bucketname-in-eu, my-bucketname-in-eu-with-object-lock

I prefer longer block in if i.e. if (!minioClient.bucketExists(...)) { is better.

balamurugana · 2020-05-12T04:49:54Z

functional/FunctionalTest.java

  public static void makeBucketwithRegion_test() throws Exception {
    if (!mintEnv) {
-      System.out.println("Test: makeBucket(String bucketName, String region)");
+      System.out.println("Test: makeBucket(MakeBucketArgs args) with region");


Add tag before the API Test: with region: makeBucket(...)

balamurugana · 2020-05-12T04:52:51Z

functional/FunctionalTest.java

@@ -303,6 +301,33 @@ public static void makeBucketWithPeriod_test() throws Exception {
    }
  }

+  /** Test: makeBucket(MakeBucketArgs args). */
+  public static void makeBucketwithRegioAndObjectLock() throws Exception {


I think we should use makeBucket_testN() like other tests.

balamurugana · 2020-05-12T15:11:29Z

build.gradle

@@ -67,6 +67,7 @@ subprojects {
        it.options.fork = true
        it.options.compilerArgs += ["-Xlint:unchecked", "-Xlint:deprecation", "-Xlint:-options"]
        it.options.encoding = "UTF-8"
+        it.options.compilerArgs << "-Werror"


Can we add "-Werror" in it.options.compilerArgs += line?

This worked
it.options.compilerArgs += ["-Xlint:unchecked", "-Xlint:deprecation", "-Xlint:-options", "-Werror"]

anjalshireesh

Looks good to me

sinhaashish force-pushed the builder-make-bucket branch 3 times, most recently from 73eeb8e to 199fb44 Compare May 11, 2020 07:43

Make bucket with builder

7f85037

sinhaashish force-pushed the builder-make-bucket branch from 199fb44 to 7f85037 Compare May 11, 2020 08:15

anjalshireesh requested changes May 11, 2020

View reviewed changes

balamurugana reviewed May 11, 2020

View reviewed changes

balamurugana reviewed May 12, 2020

View reviewed changes

sinhaashish force-pushed the builder-make-bucket branch 2 times, most recently from bbf54c3 to 076b9e7 Compare May 12, 2020 04:56

Address review comment

0542e61

sinhaashish force-pushed the builder-make-bucket branch from 076b9e7 to 0542e61 Compare May 12, 2020 04:59

Address bala's comment

92caa86

sinhaashish force-pushed the builder-make-bucket branch from 962b995 to 92caa86 Compare May 12, 2020 11:26

balamurugana reviewed May 12, 2020

View reviewed changes

Change Gradle to build error upon warning

4af1da9

sinhaashish force-pushed the builder-make-bucket branch from f60bb06 to 4af1da9 Compare May 12, 2020 15:27

anjalshireesh approved these changes May 12, 2020

View reviewed changes

balamurugana approved these changes May 12, 2020

View reviewed changes

kannappanr merged commit 7eaf152 into minio:master May 12, 2020

sinhaashish mentioned this pull request May 14, 2020

Make Bucket using builder pattern #915

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make bucket with builder #927

Make bucket with builder #927

sinhaashish commented May 11, 2020

anjalshireesh left a comment

balamurugana May 11, 2020

balamurugana May 11, 2020

balamurugana May 11, 2020

sinhaashish May 12, 2020

balamurugana May 11, 2020

balamurugana May 11, 2020

balamurugana May 11, 2020

balamurugana May 11, 2020

sinhaashish May 12, 2020

balamurugana May 11, 2020

balamurugana May 11, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

balamurugana May 12, 2020

sinhaashish May 12, 2020

anjalshireesh left a comment

		@@ -16,30 +16,55 @@

		package io.minio;

		/** Bucket Arguments to hold base bucket properties */

Make bucket with builder #927

Make bucket with builder #927

Conversation

sinhaashish commented May 11, 2020

anjalshireesh 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

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

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

anjalshireesh left a comment

Choose a reason for hiding this comment