KKcorps commented on PR #19817: URL: https://github.com/apache/pinot/pull/19817#issuecomment-6121508956
Thanks for this. The download hold looks right for tables that get past the #19710 check, like PROTECTED being turned on after the table exists, or the server-level `allowPartialUpsertConsumptionDuringCommit` default. Two asks. First, can we gate the build-time release the same way? `buildSegmentInternal` still calls `closeStreamConsumerAndReleaseSemaphore()` whenever the policy allows consumption during build. For these same tables, a failed or CRC-mismatched build then falls back to a download with the slot already free. So the revert still races the next consumer, and today that path only logs. If that release also checked `shouldRevertMetadataOnInconsistency`, these tables would behave like `DISALLOW_ALWAYS` at runtime. That is what the error message already tells people to set. Second, dedup has the same race, and this PR doesn't cover it. Take a table with `allowDedupConsumptionDuringCommit`. A replica that consumed past the committed end offset downloads with the slot released. The next consuming segment replays those rows. `checkRecordPresentOrUpdate` finds their keys still mapped to the old mutable segment, so it drops them as duplicates. Then `MutableSegmentImpl.offload()` removes those keys, and the rows end up in neither segment. Could you open a separate PR that holds the slot across the download for dedup tables too? -- 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]
