Jackie-Jiang commented on code in PR #19817:
URL: https://github.com/apache/pinot/pull/19817#discussion_r4251227018
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertContext.java:
##########
@@ -201,8 +202,11 @@ public File getTableIndexDir() {
/// during segment replacement after force commit. This happens when:
/// - Partial upsert is enabled (records need to be merged with previous
values)
/// - dropOutOfOrderRecord is enabled with NONE consistency mode (records
may have been dropped)
+ /// Pauseless tables are excluded: they consume while the previous segment
is still building, so the upsert
+ /// metadata cannot be kept reconcilable across replicas.
public boolean isTableTypeInconsistentDuringConsumption() {
- return _dropOutOfOrderRecord || _outOfOrderRecordColumn != null ||
_partialUpsertHandlerSupplier != null;
+ return (_dropOutOfOrderRecord || _outOfOrderRecordColumn != null ||
_partialUpsertHandlerSupplier != null)
+ && !PauselessConsumptionUtils.isPauselessEnabled(_tableConfig);
Review Comment:
[P1] Preserve the download barrier for pauseless partial upsert
This predicate also controls
`RealtimeSegmentDataManager.getParallelConsumptionPolicy()`. For pauseless
partial-upsert tables with no explicit policy and the legacy
allow-during-commit flag disabled, returning false changes the default from
`ALLOW_DURING_BUILD_ONLY` to `ALLOW_ALWAYS`.
A lagging replica can take the direct-download path before any local build,
for example in `DOWNLOAD` completion mode. It now releases the consumer
semaphore before loading the committed predecessor. If the local value is 10
but the committed predecessor contains 15, the successor can consume an
increment of 1 and store 11 instead of 16. Loading the predecessor afterward
cannot repair the newer merged record because its comparison value is older.
Startup readiness and predecessor registration do not block this interleaving
on an already-running table.
Please preserve the original table-type classification for policy selection
and apply the pauseless exclusion separately to revert eligibility and
previous-location tracking.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1765,16 +1765,18 @@ 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.
+ boolean revertsUpsertMetadata = _partitionUpsertMetadataManager != null
+ &&
_partitionUpsertMetadataManager.shouldRevertMetadataOnInconsistency(_realtimeSegment);
+ // Hold the semaphore when the replace reverts, so the next consuming
segment does not start against a partially
+ // reverted primary key map.
+ boolean allowedDuringDownload =
+ _parallelSegmentConsumptionPolicy.isAllowedDuringDownload() &&
!revertsUpsertMetadata;
Review Comment:
[P2] Add regression coverage for the semaphore override
The existing
`testDownloadAfterLocalBuildFlagsUpsertRevertWhenConsumingDuringBuild` cases
use only `ALLOW_DURING_BUILD_ONLY` and `DISALLOW_ALWAYS`, so neither exercises
a download-allowed policy with `revertsUpsertMetadata=true`. Removing this new
guard would leave those assertions passing.
Please add cases for `ALLOW_DURING_DOWNLOAD_ONLY` and `ALLOW_ALWAYS` that
assert the permit remains held during replacement when reverting and is
released after offload, with a no-revert control verifying that early release
is preserved.
--
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]
