ankitsultana commented on code in PR #16432:
URL: https://github.com/apache/pinot/pull/16432#discussion_r2308963792
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1205,20 +1205,25 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata
zkMetadata, IndexLoading
- The "DONE" status confirms the COMMIT_END_METADATA call succeeded,
and the segment is available either in deep storage or with a peer
before discarding the local copy.
+ The only exception is if the server does not check CRC on segment load.
Then:
We need to fall back to downloading the segment from deep storage to load
it.
*/
- if (segmentMetadata == null || (isSegmentStatusCompleted(zkMetadata) &&
!hasSameCRC(zkMetadata, segmentMetadata))) {
- if (segmentMetadata == null) {
- _logger.info("Segment: {} does not exist", segmentName);
- } else if (!hasSameCRC(zkMetadata, segmentMetadata)) {
- _logger.info("Segment: {} has CRC changed from: {} to: {}",
segmentName, segmentMetadata.getCrc(),
- zkMetadata.getCrc());
- }
+ if (segmentMetadata == null) {
+ _logger.info("Segment: {} does not exist", segmentName);
closeSegmentDirectoryQuietly(segmentDirectory);
return false;
}
+ if (isSegmentStatusCompleted(zkMetadata) && !hasSameCRC(zkMetadata,
segmentMetadata)) {
Review Comment:
@chenboat : there are a few other places where this check is being done ,
like `replaceSegmentIfCrcMismatch`. Can you check all callers of `hasSameCRC`
and make sure the config is honored in all the cases?
--
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]
