Skip to content

[ISSUE #5432] Stop fencing acks of deliveries that have no partition scope - #5434

Open
Lyin-OTTF wants to merge 2 commits into
apache:developfrom
Lyin-OTTF:fix/5432-clustered-http-ack
Open

Lyin-OTTF wants to merge 2 commits into
apache:developfrom
Lyin-OTTF:fix/5432-clustered-http-ack

Conversation

@Lyin-OTTF

Copy link
Copy Markdown
Contributor

What is the bug

ReliableDispatcher#ackDelivery runs the partition ownership guard for every delivery:

if (guard != null && !guard.test(d.getTopic(), d.getPartition())) { ... StaleOwnerException ... }

UniIngressService#deliverLocal routes a locally delivered event with partition = -1 — the "no partition scope" marker. The ownership view only ever holds real partition indexes, so owned.contains(-1) is always false and every ACK of such a delivery was rejected. Fencing is a per-partition concept; it does not apply to a delivery that has no partition.

The consequence chain:

  1. StaleOwnerException escaped the HTTP handler, so the connection was closed without any response.
  2. PushService#ack removed the callback before invoking it, so the client's retry was answered with 404 unknown deliveryId.
  3. The subscriber could neither acknowledge the event nor learn what happened, and its offset never advanced.

What the fix does

  • ReliableDispatcher — run the ownership guard only for a delivery that is bound to a partition (partition >= 0). Partition-less deliveries are not fenced.
  • PushService#ack — keep the ACK callback registered unless the ACK actually succeeds, so a failed ACK stays ackable instead of turning into a 404.
  • UniHttpServer — answer a fenced ACK with 409 including the topic/partition, instead of letting the exception escape the handler. The delivery is dropped on the fenced instance and the broker redelivers it to the new owner (POP invisibleTime), so the client sees a real answer and a retry is safe.

Tests

  • StaleOwnerFencingTest — new case: a delivery without partition scope is not fenced; the existing partition-scoped fencing cases still pass.
  • PushServiceTest#failedAckKeepsTheDeliveryAckable — a throwing ACK leaves the delivery resolvable.
  • UniHttpServerAckErrorTest (new) — ACK returns 409 for a fenced delivery, 404 for an unknown one, 200 on success.

./gradlew :eventmesh-runtime:test — 12 tests in the three classes, 0 failures; full :eventmesh-runtime:test green.

Fixes #5432

…ition scope

ReliableDispatcher#ackDelivery ran the partition-ownership guard for every
delivery. UniIngressService#deliverLocal routes a locally delivered event with
partition = -1, the "no partition scope" marker, and the ownership view only
ever holds real partition indexes, so owned.contains(-1) was always false:
every ACK of such a delivery was rejected with StaleOwnerException.

The exception then escaped the HTTP handler, so the connection was closed
without any response, and because PushService#ack removed the callback before
invoking it, the client's retry was answered with 404 "unknown deliveryId".
The subscriber could neither acknowledge the event nor find out what happened,
and its offset never advanced.

- ReliableDispatcher: run the ownership guard only when the delivery is bound
  to a partition (partition >= 0).
- PushService: keep the ACK callback registered unless the ACK succeeds, so a
  failed ACK stays ackable.
- UniHttpServer: answer a fenced ACK with 409 (topic/partition in the message)
  instead of letting the exception escape the handler.

Fixes apache#5432

Signed-off-by: Lyin-OTTF <htao08203@gmail.com>

This branch has not been deployed

No deployments
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] Clustered HTTP ACK closes the connection, then returns 404 without advancing offsets

1 participant