Praveenkumar76 commented on PR #25679: URL: https://github.com/apache/pulsar/pull/25679#issuecomment-4385174043
Thanks for the detailed explanation, @lhotari. My original change aimed to resolve the `ConditionTimeoutException` by ensuring the test never missed a snapshot signal, but I now understand that the 'lossy-but-monotonic' design is intentional for production performance. I agree that fixing the test configuration is the better approach here, I will: Revert the changes to MessageDeduplication.java. Update the test to use a lower brokerDeduplicationSnapshotIntervalSeconds as suggested to ensure frequent snapshots during verification. Update the PR title/description to reflect a test stabilization change. Pushing the update shortly. > I don't think that MessageDuplication needs to be changed. The test itself is invalid and should be fixed instead. Here's the explanation of MessageDuplication, answer from DeepWiki: > > > # `MessageDeduplication.takeSnapshot(Position)` — Role and Correctness > > ## Role of the method > > The private `takeSnapshot(Position)` in `org.apache.pulsar.broker.service.persistent.MessageDeduplication` persists the current deduplication state to a `ManagedCursor`. The snapshot captures the highest sequence IDs persisted per producer (the `highestSequencedPersisted` map) and marks the cursor at the supplied `Position`, which corresponds to the last confirmed entry in the `ManagedLedger`. This is what allows deduplication state to be recovered correctly after a broker restart or topic unload/reload. > > ## When it's invoked > > > > * Periodically, after `snapshotInterval` entries have been persisted (counter-driven, from `recordMessagePersistedNormal` / `recordMessagePersistedRepl`). > > * After replaying the cursor on a deduplication status check, if enough entries were processed. > > * After purging inactive producers, to persist the trimmed state. > > * From a scheduled task driven by `brokerDeduplicationSnapshotFrequencyInSeconds` / `brokerDeduplicationSnapshotIntervalSeconds`, via the public `takeSnapshot()` wrapper. > > * From `BacklogQuotaManager` when the deduplication cursor is the slowest consumer and needs to be advanced. > > > > ## Correctness requirements when one is already in progress > > Concurrency is gated by an `AtomicBoolean snapshotTaking`. If a snapshot is requested while another is in flight, the new request is **dropped (not queued)** and a warning is logged — there is no waiting and no replacement. The contract relies on two things to stay correct under that drop: > > > > 1. The snapshot reads `highestSequencedPersisted` at invocation time and pairs it with the supplied `Position` (last confirmed entry), so each snapshot is internally consistent. > > 2. The underlying `ManagedCursor.markDelete` is monotonic — it never moves the cursor backward — so even though concurrent requests are skipped, missing one is safe: a later snapshot will advance the cursor to a position at least as far as the dropped one would have. Combined with the periodic/scheduled triggers, the cursor is guaranteed to keep moving forward. > > > > The practical implication: callers must not assume their requested snapshot actually ran. The mechanism is designed to be lossy-but-monotonic — fine for periodic persistence, but not something to rely on for "snapshot exactly at this position right now" semantics. > > > > _Source: [DeepWiki query on apache/pulsar](https://deepwiki.com/search/in-orgapachepulsarbrokerservic_24d75d62-23c2-4a1f-a625-7d433ff820e3)_ > > One possible way to address the flaky test problem is to configure `brokerDeduplicationSnapshotIntervalSeconds` to a low value in the test instead of relying on `brokerDeduplicationEntriesInterval`. This shouldn't change the intention of the test at all. -- 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]
