Jackie-Jiang commented on code in PR #19450:
URL: https://github.com/apache/pinot/pull/19450#discussion_r4383082288
##########
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:
Could we rename the metric to `SEGMENT_BUILD_WITH_CRC_MISMATCH` and
increment it here, when `isLocalSegmentCrcMatchingZk()` returns false? This
would count a locally built segment that differs from the committed copy,
regardless of whether the fallback download succeeds.
That would let us keep `buildSegmentAndReplace()` returning boolean, remove
`BuildSegmentResult`, and remove the duplicated metric handling from both
transition branches. The `BaseTableDataManager` increments could also be
removed, since existing-segment replacements and reloads are outside this
metric's scope.
I'd keep the comparison helper free of side effects and emit in this
mismatch branch. CRC-read errors and build failures should remain uncounted; a
confirmed mismatch should count even if the subsequent download fails. The
tests should assert 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]
