tarun11Mavani commented on code in PR #19450:
URL: https://github.com/apache/pinot/pull/19450#discussion_r3946697262
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1393,56 +1400,58 @@ SegmentCompletionProtocol.Response commit(String
controllerVipUrl) {
protected boolean buildSegmentAndReplace()
throws Exception {
- return buildSegmentAndReplace(null);
+ return buildSegmentAndReplace(null) == BuildSegmentResult.SUCCESS;
}
/// Builds the segment from the in-memory rows and replaces the CONSUMING
segment with the local copy.
///
/// A locally-built segment can diverge in CRC from the one the winning
replica committed (e.g. different docId
/// ordering); swapping in a divergent copy corrupts upsert metadata and
leaves replicas inconsistent otherwise. So
- /// when `committedSegmentZKMetadata` carries a CRC, a mismatch discards the
local build (returns `false`) and the
+ /// when `committedSegmentZKMetadata` carries a CRC, a mismatch discards the
local build and the
/// caller downloads the committed segment. Skipped for pauseless tables and
segments whose CRC is not yet set (the
/// COMMITTING window), so we never download a not-yet-uploaded segment (see
PR #17885).
///
/// @param committedSegmentZKMetadata committed ZK metadata carrying the
CRC, or `null` to skip the check
- /// @return `true` if built and replaced locally; `false` if the build
failed or was rejected on a CRC mismatch
- protected boolean buildSegmentAndReplace(@Nullable SegmentZKMetadata
committedSegmentZKMetadata)
+ /// @return result indicating success, build failure, or rejection caused by
a confirmed CRC mismatch
+ protected BuildSegmentResult buildSegmentAndReplace(@Nullable
SegmentZKMetadata committedSegmentZKMetadata)
throws Exception {
SegmentBuildDescriptor descriptor;
try {
descriptor = buildSegmentInternal(false);
} catch (Exception e) {
- return false;
+ return BuildSegmentResult.FAILURE;
}
if (descriptor == null) {
- return false;
+ return BuildSegmentResult.FAILURE;
}
boolean crcCheckEnabled = committedSegmentZKMetadata != null &&
committedSegmentZKMetadata.getCrc() >= 0
&& !PauselessConsumptionUtils.isPauselessEnabled(_tableConfig);
- if (crcCheckEnabled &&
!isLocalSegmentCrcMatchingZk(committedSegmentZKMetadata)) {
- _segmentLogger.warn("Locally-built segment: {} CRC does not match
committed CRC: {} in zk. "
- + "Skipping local build to replace", _segmentNameStr,
committedSegmentZKMetadata.getCrc());
- return false;
+ if (crcCheckEnabled) {
+ try {
+ if (!isLocalSegmentCrcMatchingZk(committedSegmentZKMetadata)) {
+ _segmentLogger.warn("Locally-built segment: {} CRC does not match
committed CRC: {} in zk. "
+ + "Skipping local build to replace", _segmentNameStr,
committedSegmentZKMetadata.getCrc());
+ return BuildSegmentResult.CRC_MISMATCH;
+ }
+ } catch (Exception e) {
+ _segmentLogger.warn("Failed to read CRC of locally-built segment: {};
treating as CRC mismatch to download the "
+ + "committed segment", _segmentNameStr, e);
+ return BuildSegmentResult.FAILURE;
Review Comment:
The log message and return type don't match here.
Should we have something like "treating as a build failure to download the
committed segment"?
--
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]
