klsince commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1073106279
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -373,13 +372,31 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
} else {
LOGGER.info("Reload existing segment: {} of table: {} on tier: {}",
segmentName, _tableNameWithType,
TierConfigUtils.normalizeTierName(segmentTier));
+ SegmentDirectory segmentDirectory =
+ initSegmentDirectory(segmentName,
String.valueOf(zkMetadata.getCrc()), indexLoadingConfig);
+ if ((zkMetadata.getTier().equals(segmentTier))) {
Review Comment:
can inline the `ImmutableSegmentLoader.needPreprocess` to have one if-check
for the case of reusing segment and the log msg in the else-branch can cover
all else cases
```
if (tier is same && !ImmutableSegmentLoader.needPreprocess..) {..}
else {...}
```
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -373,13 +372,31 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
} else {
LOGGER.info("Reload existing segment: {} of table: {} on tier: {}",
segmentName, _tableNameWithType,
TierConfigUtils.normalizeTierName(segmentTier));
+ SegmentDirectory segmentDirectory =
+ initSegmentDirectory(segmentName,
String.valueOf(zkMetadata.getCrc()), indexLoadingConfig);
+ if ((zkMetadata.getTier().equals(segmentTier))) {
+ // We should first try to reuse existing segment directory
+ boolean needReprocess =
ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig,
schema);
+ if (!needReprocess) {
+ LOGGER.info(
+ "Reloading segment : {} of table : {} using existing segment
directory as no reprocessing needed",
+ segmentName, _tableNameWithType);
+ // No reprocessing needed, reuse the same segment
+ ImmutableSegment segment =
ImmutableSegmentLoader.load(segmentDirectory, indexLoadingConfig, schema);
+ addSegment(segment);
+ return;
+ } else {
+ LOGGER.info("Segment : {} of table : {} requires reprocessing
before reloading", segmentName,
+ _tableNameWithType);
+ }
+ }
+ // Create backup directory to handle failure of segment reloading.
+ createBackup(indexDir);
// The indexDir is empty after calling createBackup, as it's renamed
to a backup directory.
// The SegmentDirectory should initialize accordingly. Like for
SegmentLocalFSDirectory, it
// doesn't load anything from an empty indexDir, but gets the info to
complete the copyTo.
- try (SegmentDirectory segmentDirectory =
initSegmentDirectory(segmentName, String.valueOf(zkMetadata.getCrc()),
- indexLoadingConfig)) {
- segmentDirectory.copyTo(indexDir);
- }
+ segmentDirectory.copyTo(indexDir);
Review Comment:
to be safe, better do `try { copyTo() } finally {close()}`
--
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
