shuturmurgh commented on code in PR #19450:
URL: https://github.com/apache/pinot/pull/19450#discussion_r4432659314
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1403,56 +1416,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)) {
Review Comment:
Done:
Renamed to `SEGMENT_BUILD_WITH_CRC_MISMATCH`
Emit in `buildSegmentAndReplace` when `isLocalSegmentCrcMatchingZk()`
returns false (helper stays side-effect free)
Confirmed mismatch counts even if download fallback fails; build failures /
CRC-read errors stay uncounted
Restored boolean return, removed `BuildSegmentResult` and duplicated
transition-branch metric handling
Removed `BaseTableDataManager` increments
Tests updated for those semantics
--
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]
