[GitHub] [pinot] richardstartin commented on a change in pull request #7319: Move offline seg download code to OfflineTableManager

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

richardstartin commented on a change in pull request #7319:
URL: https://github.com/apache/pinot/pull/7319#discussion_r704600925



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
##########
@@ -256,4 +267,147 @@ public void addSegmentError(String segmentName, SegmentErrorInfo segmentErrorInf
           .collect(Collectors.toMap(map -> map.getKey().getSecond(), Map.Entry::getValue));
     }
   }
+
+  @Override
+  public void addOrReplaceSegment(String segmentName, IndexLoadingConfig indexLoadingConfig,
+      SegmentMetadata localMetadata, SegmentZKMetadata zkMetadata, boolean forceDownload)
+      throws Exception {
+    if (!forceDownload && !isNewSegment(zkMetadata, localMetadata)) {
+      LOGGER.info("Segment: {} of table: {} has crc: {} same as before, already loaded, do nothing", segmentName,
+          _tableNameWithType, localMetadata.getCrc());
+      return;
+    }
+
+    // If not forced to download, then try to recover if no local metadata is provided.
+    if (!forceDownload && localMetadata == null) {
+      LOGGER.info("Segment: {} of table: {} is not loaded, checking disk", segmentName, _tableNameWithType);
+      localMetadata = recoverSegmentQuietly(segmentName);
+      if (!isNewSegment(zkMetadata, localMetadata)) {
+        LOGGER.info("Segment: {} of table {} has crc: {} same as before, loading", segmentName, _tableNameWithType,
+            localMetadata.getCrc());
+        if (loadSegmentQuietly(segmentName, indexLoadingConfig)) {
+          return;
+        }
+        localMetadata = null;
+      }
+    }
+
+    // Download segment and replace the local one, either due to being forced to download, or the
+    // local segment is not able to get loaded, or the segment data is updated and has new CRC now.
+    if (forceDownload) {
+      LOGGER.info("Force to download segment: {} of table: {}", segmentName, _tableNameWithType);
+    } else if (localMetadata == null) {
+      LOGGER.info("Download segment: {} of table: {} as no one exists locally", segmentName, _tableNameWithType);
+    } else {
+      LOGGER.info("Download segment: {} of table: {} as local crc: {} mismatches remote crc: {}.", segmentName,
+          _tableNameWithType, localMetadata.getCrc(), zkMetadata.getCrc());
+    }
+    File indexDir = downloadSegmentFromDeepStore(segmentName, zkMetadata);
+    SegmentMetadata segmentMetadata = new SegmentMetadataImpl(indexDir);
+    addSegment(indexDir, indexLoadingConfig);
+    LOGGER.info("Downloaded and replaced segment: {} of table: {} with crc: {}", segmentName, _tableNameWithType,
+        segmentMetadata.getCrc());
+  }
+
+  /**
+   * Server restart during segment reload might leave segment directory in inconsistent state, like the index
+   * directory might not exist but segment backup directory existed. This method tries to recover from reload
+   * failure before checking the existence of the index directory and loading segment metadata from it.
+   */
+  private SegmentMetadata recoverSegmentQuietly(String segmentName) {

Review comment:
       I think naming conventions like this are helpful, and help reduce paranoid error handling. Without reading the method body itself, the author is communicating to the reader that the method doesn't throw. 
   
   If the method _does_ throw unexpectedly, it's revealing in stack traces, either in logs or in profiles, drawing one's eye to something which shouldn't have happened.




-- 
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-unsubscribe@pinot.apache.org

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