[fix][client] Guard geo-replication duplicate rewind acks#26124
[fix][client] Guard geo-replication duplicate rewind acks#26124goutamadwant wants to merge 1 commit into
Conversation
lhotari
left a comment
There was a problem hiding this comment.
Thanks for taking an interest in this and for putting together a PR against #26113.
I want to be upfront so I don't waste your time: I don't think this approach is the right fix. There are several problems with it — at a high level, guarding the duplicate ack on the client side reacts to the symptom after the destination gap has already formed, so it can't actually recover the message, and the condition it keys on is ambiguous enough that it risks stalling healthy replication. The real fix for #26113 is multi-faceted and lives mostly on the broker side (publish/dedup ordering), so it needs careful design discussion rather than a targeted client patch. I'm happy to go into the specifics if that's useful.
Beyond the technical side, a note on process: Apache Pulsar follows the ASF's generative tooling guidance (see AGENTS.md), which requires a human contributor who has reviewed, understands, and takes accountability for the change. Reviewing AI-generated solutions where a bot iterates on maintainer feedback is a real drain on maintainer time, and it isn't a workflow we can sustain. If you're a Pulsar user who's genuinely up for contributing and willing to put in the effort to understand and drive this, I'd be glad to work with you on it — the best starting point for a change this involved is a discussion on the dev@ mailing list so we can align on direction before code. If that's the case, just let me know and I'm happy to help.
Fixes #26113
Motivation
Geo-replication can rewind the source cursor after a head-of-queue send failure while later pipelined sends have already advanced the replicated deduplication watermark on the remote cluster. If the rewound head message is then acknowledged as a duplicate with target position
-1:-1, completing that send as success lets the source cursor move past an entry that was not confirmed persisted on the remote side.Modifications
GeoReplicationProducerImpltests for the failed rewind path and the success cases that should remain unchanged.Verifying this change
This change added tests and can be verified as follows:
GeoReplicationProducerImplTestcoverage for rewound duplicate acknowledgements with no target position, valid target positions, and duplicate acknowledgements at the current persisted source position../gradlew :pulsar-client-original:test --tests "org.apache.pulsar.client.impl.GeoReplicationProducerImplTest" -PtestRetryCount=0 --no-build-cache --rerun-tasks./gradlew :pulsar-client-original:spotlessCheck :pulsar-client-original:checkstyleMain :pulsar-client-original:checkstyleTest./gradlew quickCheckDoes this pull request potentially affect one of the following parts:
This changes a geo-replication failure path from completing a duplicate rewind acknowledgement as success to failing the send callback, so the source cursor does not advance past an entry that was not confirmed persisted on the remote side.