Skip to content

[fix][client] Guard geo-replication duplicate rewind acks#26124

Open
goutamadwant wants to merge 1 commit into
apache:masterfrom
goutamadwant:fix-geo-repl-dedup-ack
Open

[fix][client] Guard geo-replication duplicate rewind acks#26124
goutamadwant wants to merge 1 commit into
apache:masterfrom
goutamadwant:fix-geo-repl-dedup-ack

Conversation

@goutamadwant

Copy link
Copy Markdown

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

  • Treat a duplicate acknowledgement without a target position as a failed send when the rewound source position is earlier than the last persisted source position.
  • Keep the existing success path for valid target positions and for duplicate acknowledgements at the current persisted source position.
  • Add focused GeoReplicationProducerImpl tests for the failed rewind path and the success cases that should remain unchanged.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added GeoReplicationProducerImplTest coverage 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 quickCheck

Does this pull request potentially affect one of the following parts:

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

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.

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Geo-replication can silently drop a message when a head-of-queue send failure rewinds the cursor after later pipelined messages already persisted

2 participants