xiangfu0 commented on code in PR #19817:
URL: https://github.com/apache/pinot/pull/19817#discussion_r4280452726
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1765,26 +1766,46 @@ public void goOnlineFromConsuming(SegmentZKMetadata
segmentZKMetadata)
protected void downloadSegmentAndReplace(SegmentZKMetadata segmentZKMetadata)
throws Exception {
- boolean allowedDuringDownload =
_parallelSegmentConsumptionPolicy.isAllowedDuringDownload();
// A local build that failed or mismatched the committed CRC has already
released the semaphore when the policy
// allows consumption during build, so the next consuming segment can be
running during this download as well
boolean releasedDuringBuild =
_parallelSegmentConsumptionPolicy.isAllowedDuringBuild() &&
!_consumerSemaphoreAcquired.get();
- if ((allowedDuringDownload || releasedDuringBuild) &&
_partitionUpsertMetadataManager != null
- &&
_partitionUpsertMetadataManager.shouldRevertMetadataOnInconsistency(_realtimeSegment))
{
- // Table config validation rejects this in PROTECTED mode. It still
happens when the mode is switched after the
- // table is created, when the server-level default allows partial upsert
consumption during commit, or when a
- // pauseless table falls back to a download after its local build fails.
- _segmentLogger.error("Next consuming segment can run during download
while this replace reverts upsert "
- + "metadata. Its snapshot can miss the rows the revert restores. Use
DISALLOW_ALWAYS");
- _serverMetrics.addMeteredTableValue(_clientId,
ServerMeter.UPSERT_REVERT_WITH_CONSUMPTION_DURING_DOWNLOAD, 1L);
+ // Hold the semaphore when the replace rewrites the key map, so the next
consuming segment does not start against
+ // a partially reverted one, and does not replay rows against keys this
replace is about to remove.
+ boolean holdsConsumerSemaphore = holdsConsumerSemaphoreForMetadata();
+ boolean allowedDuringDownload =
+ _parallelSegmentConsumptionPolicy.isAllowedDuringDownload() &&
!holdsConsumerSemaphore;
+ if (holdsConsumerSemaphore && (releasedDuringBuild
+ || _parallelSegmentConsumptionPolicy.isAllowedDuringDownload())) {
+ _segmentLogger.warn("Policy {} would let the next consuming segment run
while this replace rewrites upsert or "
Review Comment:
Minor (non-blocking): merging the two cases means the genuinely at-risk path
(`releasedDuringBuild` — semaphore already gone, rows can actually be dropped)
now logs at WARN with the same text as the harmless "held against policy"
over-hold. The metric below still isolates the dangerous case, but an operator
reading logs loses that distinction. Consider keeping ERROR + the "its rows can
be dropped against keys this replace removes" wording when
`releasedDuringBuild`, and WARN for the policy-only case.
--
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]
