Jackie-Jiang commented on code in PR #9306:
URL: https://github.com/apache/pinot/pull/9306#discussion_r979075807
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -347,19 +349,21 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
}
indexDir = downloadSegment(segmentName, zkMetadata);
} else {
- LOGGER.info("Reload existing segment: {} of table: {}", segmentName,
_tableNameWithType);
+ LOGGER.info("Reload existing segment: {} of table: {} currently on
tier: {}", segmentName, _tableNameWithType,
Review Comment:
(minor) In most of the cases, tier will be `null`. Consider adding an if
check instead of always logging the tier info
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -347,19 +349,21 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
}
indexDir = downloadSegment(segmentName, zkMetadata);
} else {
- LOGGER.info("Reload existing segment: {} of table: {}", segmentName,
_tableNameWithType);
+ LOGGER.info("Reload existing segment: {} of table: {} currently on
tier: {}", segmentName, _tableNameWithType,
+ segmentTier);
// 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, schema)) {
+ segmentTier, indexLoadingConfig, schema)) {
segmentDirectory.copyTo(indexDir);
}
}
// Load from indexDir and replace the old segment in memory. What's
inside indexDir
// may come from SegmentDirectory.copyTo() or the segment downloaded
from deep store.
- ImmutableSegment segment = ImmutableSegmentLoader.load(indexDir,
indexLoadingConfig, schema);
+ ImmutableSegment segment =
Review Comment:
Consider wrapping `tableDataDir` and `tier` into the `indexLoadingConfig` to
reduce the parameters passed.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -329,7 +330,8 @@ public Map<String, SegmentErrorInfo> getSegmentErrors() {
public void reloadSegment(String segmentName, IndexLoadingConfig
indexLoadingConfig, SegmentZKMetadata zkMetadata,
SegmentMetadata localMetadata, @Nullable Schema schema, boolean
forceDownload)
throws Exception {
- File indexDir = getSegmentDataDir(segmentName);
+ String segmentTier = getSegmentCurrentTier(segmentName);
Review Comment:
We should store the tier name within the `localMetadata`. You should be able
to set it within the `ImmutableSegmentLoader.load(SegmentDirectory
segmentDirectory, IndexLoadingConfig indexLoadingConfig, @Nullable Schema
schema)`
--
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]
