You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/11/17 13:47:22 UTC

[GitHub] [incubator-pinot] Aka-shi opened a new pull request #6272: Set S3 Bucket ACL policy from config

Aka-shi opened a new pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272


   ## Description
   This PR introduces a config key to decide either to grant full control to bucket(aws s3) owner or not. These will be useful when the pinot-cluster and S3 bucket are in different AWS accounts. 
   
   Here is the issue related to this. #6161 . 
   
   ## Documentation
   Raised a [PR](https://github.com/pinot-contrib/pinot-docs/pull/15) to include it in the pinot-docs specific to S3. 
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525642646



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -72,17 +70,19 @@
   public static final String SECRET_KEY = "secretKey";
   public static final String REGION = "region";
   public static final String ENDPOINT = "endpoint";
+  public static final String DISABLE_ACL = "disableAcl";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3PinotFS.class);
   private static final String DELIMITER = "/";
   public static final String S3_SCHEME = "s3://";
   private S3Client _s3Client;
+  private Boolean disableAcl = true;
 
   @Override
   public void init(PinotConfiguration config) {
     Preconditions.checkArgument(!isNullOrEmpty(config.getProperty(REGION)));
     String region = config.getProperty(REGION);
-
+    disableAcl = config.getProperty(DISABLE_ACL, true);

Review comment:
       Create constant  `DEFAULT_DISABLE_ACL=true` and use it here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Aka-shi commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
Aka-shi commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525731729



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -72,17 +70,19 @@
   public static final String SECRET_KEY = "secretKey";
   public static final String REGION = "region";
   public static final String ENDPOINT = "endpoint";
+  public static final String DISABLE_ACL = "disableAcl";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3PinotFS.class);
   private static final String DELIMITER = "/";
   public static final String S3_SCHEME = "s3://";
   private S3Client _s3Client;
+  private Boolean disableAcl = true;
 
   @Override
   public void init(PinotConfiguration config) {
     Preconditions.checkArgument(!isNullOrEmpty(config.getProperty(REGION)));
     String region = config.getProperty(REGION);
-
+    disableAcl = config.getProperty(DISABLE_ACL, true);

Review comment:
       Done. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525641753



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -72,17 +70,19 @@
   public static final String SECRET_KEY = "secretKey";
   public static final String REGION = "region";
   public static final String ENDPOINT = "endpoint";
+  public static final String DISABLE_ACL = "disableAcl";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3PinotFS.class);
   private static final String DELIMITER = "/";
   public static final String S3_SCHEME = "s3://";
   private S3Client _s3Client;
+  private Boolean disableAcl = true;

Review comment:
       boolean?

##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -72,17 +70,19 @@
   public static final String SECRET_KEY = "secretKey";
   public static final String REGION = "region";
   public static final String ENDPOINT = "endpoint";
+  public static final String DISABLE_ACL = "disableAcl";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3PinotFS.class);
   private static final String DELIMITER = "/";
   public static final String S3_SCHEME = "s3://";
   private S3Client _s3Client;
+  private Boolean disableAcl = true;

Review comment:
       boolean




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Aka-shi commented on pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
Aka-shi commented on pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#issuecomment-728939219


   @fx19880617 @pradeepgv42 can you help me by reviewing this PR?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525578569



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -225,9 +225,15 @@ private boolean copyFile(URI srcUri, URI dstUri)
       }
 
       String dstPath = sanitizePath(dstUri.getPath());
-      CopyObjectRequest copyReq =
-          CopyObjectRequest.builder().copySource(encodedUrl).destinationBucket(dstUri.getHost()).destinationKey(dstPath)
-              .build();
+      CopyObjectRequest.Builder copyReqBuilder = CopyObjectRequest.builder().copySource(encodedUrl)
+          .destinationBucket(dstUri.getHost()).destinationKey(dstPath);
+      CopyObjectRequest copyReq;
+
+      if (!disableAcl) {

Review comment:
       ```
   if (!disableAcl) {
       copyReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL);
   } 
   CopyObjectRequest copyReq = copyReqBuilder.build();
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Aka-shi commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
Aka-shi commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525638959



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -225,9 +225,15 @@ private boolean copyFile(URI srcUri, URI dstUri)
       }
 
       String dstPath = sanitizePath(dstUri.getPath());
-      CopyObjectRequest copyReq =
-          CopyObjectRequest.builder().copySource(encodedUrl).destinationBucket(dstUri.getHost()).destinationKey(dstPath)
-              .build();
+      CopyObjectRequest.Builder copyReqBuilder = CopyObjectRequest.builder().copySource(encodedUrl)
+          .destinationBucket(dstUri.getHost()).destinationKey(dstPath);
+      CopyObjectRequest copyReq;
+
+      if (!disableAcl) {

Review comment:
       Done. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#issuecomment-728975852


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=h1) Report
   > Merging [#6272](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=desc) (79fc19a) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.82%`.
   > The diff coverage is `67.60%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6272/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6272      +/-   ##
   ==========================================
   + Coverage   66.44%   73.27%   +6.82%     
   ==========================================
     Files        1075     1243     +168     
     Lines       54773    58949    +4176     
     Branches     8168     8739     +571     
   ==========================================
   + Hits        36396    43196    +6800     
   + Misses      15700    12913    -2787     
   - Partials     2677     2840     +163     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `46.36% <51.07%> (?)` | |
   | unittests | `64.14% <43.81%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `78.57% <ø> (+5.40%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | [.../pinot/common/function/DateTimePatternHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
   | [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
   | ... and [1019 more](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=footer). Last update [c8d7efc...c738c91](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#issuecomment-728975852


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=h1) Report
   > Merging [#6272](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=desc) (79fc19a) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `2.30%`.
   > The diff coverage is `43.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6272/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6272      +/-   ##
   ==========================================
   - Coverage   66.44%   64.14%   -2.31%     
   ==========================================
     Files        1075     1243     +168     
     Lines       54773    58949    +4176     
     Branches     8168     8739     +571     
   ==========================================
   + Hits        36396    37810    +1414     
   - Misses      15700    18384    +2684     
   - Partials     2677     2755      +78     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.14% <43.81%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | ... and [1083 more](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=footer). Last update [c8d7efc...c738c91](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525578569



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -225,9 +225,15 @@ private boolean copyFile(URI srcUri, URI dstUri)
       }
 
       String dstPath = sanitizePath(dstUri.getPath());
-      CopyObjectRequest copyReq =
-          CopyObjectRequest.builder().copySource(encodedUrl).destinationBucket(dstUri.getHost()).destinationKey(dstPath)
-              .build();
+      CopyObjectRequest.Builder copyReqBuilder = CopyObjectRequest.builder().copySource(encodedUrl)
+          .destinationBucket(dstUri.getHost()).destinationKey(dstPath);
+      CopyObjectRequest copyReq;
+
+      if (!disableAcl) {

Review comment:
       if (!disableAcl) {
       copyReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL);
   } 
   CopyObjectRequest copyReq = copyReqBuilder.build();




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525578793



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -248,7 +254,14 @@ public boolean mkdir(URI uri)
         return true;
       }
 
-      PutObjectRequest putObjectRequest = PutObjectRequest.builder().bucket(uri.getHost()).key(path).build();
+      PutObjectRequest.Builder putReqBuilder = PutObjectRequest.builder().bucket(uri.getHost()).key(path);
+      PutObjectRequest putObjectRequest;

Review comment:
       same above




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io commented on pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#issuecomment-728975852


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=h1) Report
   > Merging [#6272](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=desc) (23f96f2) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `20.16%`.
   > The diff coverage is `51.20%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6272/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #6272       +/-   ##
   ===========================================
   - Coverage   66.44%   46.28%   -20.17%     
   ===========================================
     Files        1075     1243      +168     
     Lines       54773    58949     +4176     
     Branches     8168     8739      +571     
   ===========================================
   - Hits        36396    27285     -9111     
   - Misses      15700    29424    +13724     
   + Partials     2677     2240      -437     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `46.28% <51.20%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `26.66% <0.00%> (-30.48%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `22.22% <0.00%> (-26.62%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | ... and [1232 more](https://codecov.io/gh/apache/incubator-pinot/pull/6272/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=footer). Last update [c8d7efc...23f96f2](https://codecov.io/gh/apache/incubator-pinot/pull/6272?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525641753



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -72,17 +70,19 @@
   public static final String SECRET_KEY = "secretKey";
   public static final String REGION = "region";
   public static final String ENDPOINT = "endpoint";
+  public static final String DISABLE_ACL = "disableAcl";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3PinotFS.class);
   private static final String DELIMITER = "/";
   public static final String S3_SCHEME = "s3://";
   private S3Client _s3Client;
+  private Boolean disableAcl = true;

Review comment:
       do we need to set the default value ?
   also use boolean not Boolean




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 merged pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525582760



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -491,17 +511,32 @@ public boolean touch(URI uri)
       String path = sanitizePath(uri.getPath());
       Map<String, String> mp = new HashMap<>();
       mp.put("lastModified", String.valueOf(System.currentTimeMillis()));
-      CopyObjectRequest request =
-          CopyObjectRequest.builder().copySource(encodedUrl).destinationBucket(uri.getHost()).destinationKey(path)
-              .metadata(mp).metadataDirective(MetadataDirective.REPLACE).build();
+      CopyObjectRequest.Builder copyReqBuilder = CopyObjectRequest.builder().copySource(encodedUrl)
+          .destinationBucket(uri.getHost()).destinationKey(path)
+          .metadata(mp).metadataDirective(MetadataDirective.REPLACE);
+      CopyObjectRequest request;
+
+      if (!disableAcl) {
+        request = copyReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL).build();
+      } else {
+        request = copyReqBuilder.build();
+      }
 
       _s3Client.copyObject(request);
       long newUpdateTime = getS3ObjectMetadata(uri).lastModified().toEpochMilli();
       return newUpdateTime > s3ObjectMetadata.lastModified().toEpochMilli();
     } catch (NoSuchKeyException e) {
       String path = sanitizePath(uri.getPath());
-      _s3Client.putObject(PutObjectRequest.builder().bucket(uri.getHost()).key(path).build(),
-          RequestBody.fromBytes(new byte[0]));
+      PutObjectRequest.Builder putReqBuilder = PutObjectRequest.builder().bucket(uri.getHost()).key(path);
+      PutObjectRequest putObjectRequest;
+
+      if (!disableAcl) {
+        putObjectRequest = putReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL).build();

Review comment:
       just set builder here and build the request later.

##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -491,17 +511,32 @@ public boolean touch(URI uri)
       String path = sanitizePath(uri.getPath());
       Map<String, String> mp = new HashMap<>();
       mp.put("lastModified", String.valueOf(System.currentTimeMillis()));
-      CopyObjectRequest request =
-          CopyObjectRequest.builder().copySource(encodedUrl).destinationBucket(uri.getHost()).destinationKey(path)
-              .metadata(mp).metadataDirective(MetadataDirective.REPLACE).build();
+      CopyObjectRequest.Builder copyReqBuilder = CopyObjectRequest.builder().copySource(encodedUrl)
+          .destinationBucket(uri.getHost()).destinationKey(path)
+          .metadata(mp).metadataDirective(MetadataDirective.REPLACE);
+      CopyObjectRequest request;
+
+      if (!disableAcl) {
+        request = copyReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL).build();

Review comment:
       set builder here and build the request later.

##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -445,7 +458,14 @@ public void copyFromLocalFile(File srcFile, URI dstUri)
     LOGGER.info("Copy {} from local to {}", srcFile.getAbsolutePath(), dstUri);
     URI base = getBase(dstUri);
     String prefix = sanitizePath(base.relativize(dstUri).getPath());
-    PutObjectRequest putObjectRequest = PutObjectRequest.builder().bucket(dstUri.getHost()).key(prefix).build();
+    PutObjectRequest.Builder putReqBuilder = PutObjectRequest.builder().bucket(dstUri.getHost()).key(prefix);
+    PutObjectRequest putObjectRequest;
+
+    if (!disableAcl) {
+      putObjectRequest = putReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL).build();

Review comment:
       set builder here and build the request later.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Aka-shi commented on a change in pull request #6272: Set S3 Bucket ACL policy from config

Posted by GitBox <gi...@apache.org>.
Aka-shi commented on a change in pull request #6272:
URL: https://github.com/apache/incubator-pinot/pull/6272#discussion_r525697186



##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -72,17 +70,19 @@
   public static final String SECRET_KEY = "secretKey";
   public static final String REGION = "region";
   public static final String ENDPOINT = "endpoint";
+  public static final String DISABLE_ACL = "disableAcl";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3PinotFS.class);
   private static final String DELIMITER = "/";
   public static final String S3_SCHEME = "s3://";
   private S3Client _s3Client;
+  private Boolean disableAcl = true;

Review comment:
       If I don't set the default here, the test case fail because the `init` method used to set this config and the method used in the test cases is different. In the recent commit, I am also reusing this to set the default value in `getProperty` method. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org