[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #5764: Add retry logic to download segment tar file in pinot server

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

jackjlli commented on a change in pull request #5764:
URL: https://github.com/apache/incubator-pinot/pull/5764#discussion_r461924349



##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentFetcherAndLoader.java
##########
@@ -184,39 +185,44 @@ private boolean isNewSegmentMetadata(String tableNameWithType, OfflineSegmentZKM
 
   private String downloadSegmentToLocal(String uri, PinotCrypter crypter, String tableName, String segmentName)
       throws Exception {
-    File tempDir = new File(new File(_instanceDataManager.getSegmentFileDirectory(), tableName),
-        "tmp-" + segmentName + "-" + UUID.randomUUID());
-    FileUtils.forceMkdir(tempDir);
-    File tempDownloadFile = new File(tempDir, segmentName + ENCODED_SUFFIX);
-    File tempTarFile = new File(tempDir, segmentName + TAR_GZ_SUFFIX);
-    File tempSegmentDir = new File(tempDir, segmentName);
-    try {
-      SegmentFetcherFactory.fetchSegmentToLocal(uri, tempDownloadFile);
-      if (crypter != null) {
-        crypter.decrypt(tempDownloadFile, tempTarFile);
-      } else {
-        tempTarFile = tempDownloadFile;
-      }
-
-      LOGGER
-          .info("Downloaded tarred segment: {} for table: {} from: {} to: {}, file length: {}", segmentName, tableName,
-              uri, tempTarFile, tempTarFile.length());
+    // Even if the tar file has been downloaded successfully, the file itself could be corrupted during the transmission.
+    // Thus, we should re-download it again.
+    RetryPolicies.fixedDelayRetryPolicy(5, 5_000L).attempt(() -> {
+      File tempDir = new File(new File(_instanceDataManager.getSegmentFileDirectory(), tableName),
+          "tmp-" + segmentName + "-" + UUID.randomUUID());
+      FileUtils.forceMkdir(tempDir);
+      File tempDownloadFile = new File(tempDir, segmentName + ENCODED_SUFFIX);
+      File tempTarFile = new File(tempDir, segmentName + TAR_GZ_SUFFIX);
+      File tempSegmentDir = new File(tempDir, segmentName);
+      try {
+        SegmentFetcherFactory.fetchSegmentToLocal(uri, tempDownloadFile);

Review comment:
       Comment added. 
   I also added the catch block to check whether `AttemptsExceededException` is thrown from `SegmentFetcherFactory.fetchSegmentToLocal(uri, tempDownloadFile)`. If so, exit the outer retry.




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