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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
