klsince commented on code in PR #12936:
URL: https://github.com/apache/pinot/pull/12936#discussion_r1567833872


##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -693,27 +693,22 @@ File downloadAndDecrypt(String segmentName, 
SegmentZKMetadata zkMetadata, File t
     }
   }
 
-  // not thread safe. Caller should invoke it with safe concurrency control.
   protected void downloadFromPeersWithoutStreaming(String segmentName, 
SegmentZKMetadata zkMetadata, File destTarFile)
       throws Exception {
-    Preconditions.checkState(_peerDownloadScheme != null, "Download peers 
require non null peer download scheme");
-    List<URI> peerSegmentURIs =
-        PeerServerSegmentFinder.getPeerServerURIs(_helixManager, 
_tableNameWithType, segmentName, _peerDownloadScheme);
-    if (peerSegmentURIs.isEmpty()) {
-      String msg = String.format("segment %s doesn't have any peers", 
segmentName);
-      LOGGER.warn(msg);
-      // HelixStateTransitionHandler would catch the runtime exception and 
mark the segment state as Error
-      throw new RuntimeException(msg);
-    }
+    Preconditions.checkState(_peerDownloadScheme != null, "Peer download is 
not enabled for table: %s",
+        _tableNameWithType);
     try {
-      // Next download the segment from a randomly chosen server using 
configured scheme.
-      SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(peerSegmentURIs, 
destTarFile, zkMetadata.getCrypterName());
-      LOGGER.info("Fetched segment {} from peers: {} to: {} of size: {}", 
segmentName, peerSegmentURIs, destTarFile,
+      SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(segmentName, 
_peerDownloadScheme, () -> {
+        List<URI> peerServerURIs =
+            PeerServerSegmentFinder.getPeerServerURIs(_helixManager, 
_tableNameWithType, segmentName,
+                _peerDownloadScheme);
+        Collections.shuffle(peerServerURIs);
+        return peerServerURIs;
+      }, destTarFile, zkMetadata.getCrypterName());
+      _logger.info("Downloaded tarred segment: {} from peers to: {}, file 
length: {}", segmentName, destTarFile,

Review Comment:
   nit: LOGGER or _logger?
   
   it it possible to get which peer the data was downloaded from and log it out 
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.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

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