[GitHub] [incubator-pinot] KKcorps commented on a change in pull request #6565: Support S3PinotFS with server side encryption mode 'aws:kms'

[GitBox]

KKcorps commented on a change in pull request #6565:
URL: https://github.com/apache/incubator-pinot/pull/6565#discussion_r573621982



##########
File path: 
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -526,36 +538,56 @@ public boolean touch(URI uri)
       }
 
       String path = sanitizePath(uri.getPath());
-      Map<String, String> mp = new HashMap<>();
-      mp.put("lastModified", String.valueOf(System.currentTimeMillis()));
-      CopyObjectRequest.Builder copyReqBuilder = 
CopyObjectRequest.builder().copySource(encodedUrl)
-          .destinationBucket(uri.getHost()).destinationKey(path)
-          .metadata(mp).metadataDirective(MetadataDirective.REPLACE);
-
-      if (!disableAcl) {
-        copyReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL);
-      }
-
-      CopyObjectRequest request = copyReqBuilder.build();
+      CopyObjectRequest request = generateCopyObjectRequest(encodedUrl, uri, 
path,
+          ImmutableMap.of("lastModified", 
String.valueOf(System.currentTimeMillis())));
       _s3Client.copyObject(request);
       long newUpdateTime = 
getS3ObjectMetadata(uri).lastModified().toEpochMilli();
       return newUpdateTime > s3ObjectMetadata.lastModified().toEpochMilli();
     } catch (NoSuchKeyException e) {
       String path = sanitizePath(uri.getPath());
-      PutObjectRequest.Builder putReqBuilder = 
PutObjectRequest.builder().bucket(uri.getHost()).key(path);
-
-      if (!disableAcl) {
-        putReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL);
-      }
-
-      PutObjectRequest putObjectRequest = putReqBuilder.build();
+      PutObjectRequest putObjectRequest = generatePutObjectRequest(uri, path);
       _s3Client.putObject(putObjectRequest, RequestBody.fromBytes(new 
byte[0]));
       return true;
     } catch (S3Exception e) {
       throw new IOException(e);
     }
   }
 
+  private PutObjectRequest generatePutObjectRequest(URI uri, String path) {
+    PutObjectRequest.Builder putReqBuilder = 
PutObjectRequest.builder().bucket(uri.getHost()).key(path);
+
+    if (!_disableAcl) {
+      putReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL);
+    }
+
+    if (_serverSideEncryption != null) {
+      
putReqBuilder.serverSideEncryption(_serverSideEncryption).ssekmsKeyId(_ssekmsKeyId);
+      if (_ssekmsEncryptionContext != null) {
+        putReqBuilder.ssekmsEncryptionContext(_ssekmsEncryptionContext);
+      }
+    }
+    return putReqBuilder.build();
+  }
+
+  private CopyObjectRequest generateCopyObjectRequest(String copySource, URI 
dest, String path,
+      Map<String, String> metadata) {
+    CopyObjectRequest.Builder copyReqBuilder =
+        
CopyObjectRequest.builder().copySource(copySource).destinationBucket(dest.getHost()).destinationKey(path);
+    if (metadata != null) {
+      
copyReqBuilder.metadata(metadata).metadataDirective(MetadataDirective.REPLACE);
+    }
+    if (!_disableAcl) {
+      copyReqBuilder.acl(ObjectCannedACL.BUCKET_OWNER_FULL_CONTROL);

Review comment:
       Do we require Full control ACL over here ?

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

Review comment:
       Do we require Full control ACL over 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:
us...@infra.apache.org



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