klsince commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1070055592
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -353,15 +354,37 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
indexLoadingConfig.setSegmentTier(segmentTier);
indexLoadingConfig.setTableDataDir(_tableDataDir);
indexLoadingConfig.setInstanceTierConfigs(_tableDataManagerConfig.getInstanceTierConfigs());
+ indexLoadingConfig.setSegmentTier(zkMetadata.getTier());
Review Comment:
This change may be problematic. At L354, `setSegmentTier(segmentTier)` is
called in order to create the SegmentDirectory object with current tier to kick
off the following segment reloading logic.
At L386, `indexLoadingConfig.setSegmentTier(zkMetadata.getTier());` is
called in order to load the segment with the target tier from the segment ZK
metadata to finish segment loading. Different tiers may have different configs,
for example `TierBasedSegmentDirectoryLoader` moves segments across data paths
as set in tierConfigs, based on which tier is provided via this
`setSegmentTier`.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -353,15 +354,37 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
indexLoadingConfig.setSegmentTier(segmentTier);
indexLoadingConfig.setTableDataDir(_tableDataDir);
indexLoadingConfig.setInstanceTierConfigs(_tableDataManagerConfig.getInstanceTierConfigs());
+ indexLoadingConfig.setSegmentTier(zkMetadata.getTier());
File indexDir = getSegmentDataDir(segmentName, segmentTier,
indexLoadingConfig.getTableConfig());
try {
+
+ boolean shouldDownload = forceDownload || !hasSameCRC(zkMetadata,
localMetadata);
+
+ if (!shouldDownload) {
+ // We should first try to reuse directory
+ Properties loaderContextProps = new Properties();
+
loaderContextProps.put(CommonConstants.Server.DIRECTORY_LOADER_PURPOSE_CONFIG,
+ CommonConstants.Server.DIRECTORY_LOADER_PURPOSE_RELOAD);
+ SegmentDirectory segmentDirectory =
+ initSegmentDirectory(segmentName,
String.valueOf(zkMetadata.getCrc()), indexLoadingConfig,
+ loaderContextProps);
+ boolean needReprocess =
ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig,
schema);
+ if (!needReprocess) {
+ // No reprocessing needed, reuse the same segment
+ ImmutableSegment segment =
ImmutableSegmentLoader.load(segmentDirectory, indexLoadingConfig, schema);
+ addSegment(segment);
+ return;
+ } else {
Review Comment:
looks like, if it takes the else-branch, the SegmentDirectory object may be
created again at L402.
I wonder if the changes can be simplified by checking `needPreprocess` after
segmentDirectory is created at L402. If no need to preprocess, just add segment
and done.
--
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
