[GitHub] [incubator-pinot] chenboat commented on a change in pull request #3849: Using PinotFS interface instead of local file to download a segment f…

[GitBox <gi...@apache.org>]

chenboat commented on a change in pull request #3849: Using PinotFS interface instead of local file to download a segment f…
URL: https://github.com/apache/incubator-pinot/pull/3849#discussion_r271981037
 
 

 ##########
 File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadRestletResource.java
 ##########
 @@ -182,19 +186,34 @@ public Response downloadSegment(
       throw new ControllerApplicationException(LOGGER, e.getMessage(), Response.Status.INTERNAL_SERVER_ERROR, e);
     }
     try {
-      segmentName = URLDecoder.decode(segmentName, "UTF-8");
+      segmentName = URLDecoder.decode(segmentName, URL_ENCODING_SCHEME);
     } catch (UnsupportedEncodingException e) {
       String errStr = "Could not decode segment name '" + segmentName + "'";
       throw new ControllerApplicationException(LOGGER, errStr, Response.Status.BAD_REQUEST);
     }
-    final File dataFile = new File(provider.getBaseDataDir(), StringUtil.join("/", tableName, segmentName));
-    if (!dataFile.exists()) {
+
+    final java.net.URI segmentFileURI =
+        ControllerConf.getUriFromPath(StringUtil.join("/", provider.getBaseDataDirURI().toString(),
+            tableName, URLEncoder.encode(segmentName, URL_ENCODING_SCHEME)));
+    PinotFS pinotFS = PinotFSFactory.create(provider.getBaseDataDirURI().getScheme());
+
+    if (!pinotFS.exists(segmentFileURI)) {
       throw new ControllerApplicationException(LOGGER,
-          "Segment " + segmentName + " or table " + tableName + " not found", Response.Status.NOT_FOUND);
+          "Segment " + segmentName + " or table " + tableName + " not found in " + segmentFileURI.toString(), Response.Status.NOT_FOUND);
     }
-    Response.ResponseBuilder builder = Response.ok(dataFile);
-    builder.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=" + dataFile.getName());
-    builder.header(HttpHeaders.CONTENT_LENGTH, dataFile.length());
+    File tmpSegmentFile = new File(StringUtil.join("/", _controllerConf.getLocalTempDir(), tableName,
+        StringUtil.join("_", segmentName, String.valueOf(System.nanoTime()))));
+    pinotFS.copyToLocalFile(segmentFileURI, tmpSegmentFile);
 
 Review comment:
   There will be an extra copy operation. But the tmp file will be deleted before this method exits though. One optimization I can think of is to optimize for the local FS case: there is no need for the copy op. For deep storage, I do not think of an easy way except the structural you suggested below (which i have some pushback too). 

----------------------------------------------------------------
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


With regards,
Apache Git Services

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