mayankshriv commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r445723995



##########
File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -303,46 +335,37 @@ private String getZkDownloadURIForSegmentUpload(String 
rawTableName, String segm
     }
   }
 
-  private SegmentMetadata getMetadataForURI(String crypterClassHeader, String 
currentSegmentLocationURI,
-      File tempEncryptedFile, File tempDecryptedFile, File tempSegmentDir, 
String metadataProviderClass)
+  private void getSegmentFileFromURI(String currentSegmentLocationURI, File 
destFile)

Review comment:
       A `get` method returning void seems odd to me. Probably `s/get/download`?

##########
File path: 
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java
##########
@@ -80,6 +80,14 @@ public String getActualColumnName(String tableName, String 
columnName) {
     return columnName;
   }
 
+  public TableConfig getTableConfig(String tableName) {
+    return 
_tableConfigChangeListener._tableConfigMap.get(canonicalize(tableName));
+  }
+
+  private String canonicalize(String name) {

Review comment:
       If latter, probably not worth creating another layer of method call.

##########
File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -251,6 +247,20 @@ private SuccessResponse uploadSegment(@Nullable String 
tableName, FormDataMultiP
           _controllerMetrics, 
_leadControllerManager.isLeaderForTable(offlineTableName))
           .validateOfflineSegment(offlineTableName, segmentMetadata, 
tempSegmentDir);
 
+      Pair<Boolean, String> encryptionInfo =
+          encryptSegmentIfNeeded(offlineTableName, tempDecryptedFile, 
tempEncryptedFile, uploadedSegmentIsEncrypted);

Review comment:
       crypterClassName should be an input to this method, not a return value? 
This method should just return the 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:
us...@infra.apache.org



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

Reply via email to