xiangfu0 commented on code in PR #19817:
URL: https://github.com/apache/pinot/pull/19817#discussion_r4290442686
##########
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:
Under `ALLOW_DURING_DOWNLOAD_ONLY`, `holdsConsumerSemaphore` is true so
`allowedDuringDownload` was just set to false and we hold the permit through
the download. But this warn still fires on that same path
(`isAllowedDuringDownload()` is true), telling operators the next segment
"would run" and to switch to `DISALLOW_ALWAYS` — even though the code already
made this replace safe by holding.
That means every upsert-revert/dedup download under this policy logs a
misleading warning. Consider only warning when the semaphore is actually
released here (e.g. gate on `allowedDuringDownload || releasedDuringBuild`).
--
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]
