Skip to content

fix(#5295): gate RocketMQ 5 POP broker ACK on distribution completion (rebased from #5316) - #5330

Merged
qqeasonchen merged 1 commit into
apache:developfrom
qqeasonchen:merge-5316-pop-broker-ack
Sep 7, 2026
Merged

fix(#5295): gate RocketMQ 5 POP broker ACK on distribution completion (rebased from #5316)#5330
qqeasonchen merged 1 commit into
apache:developfrom
qqeasonchen:merge-5316-pop-broker-ack

Conversation

@qqeasonchen

@qqeasonchen qqeasonchen commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request

Fix #5295: Gate RocketMQ 5 POP broker ACK on distribution completion.

Problem

Previously, a single mqAck callback was shared across all deliveries of a frame. In BROADCAST/MULTICAST mode, the first client ACK would immediately ACK the broker, even if other required targets had not yet received or acknowledged the message.

Solution

Introduce a broker-ACK barrier using an AtomicInteger counter:

  • All deliveries of the same frame share a single counter initialized to targets.size()
  • Broker ACK fires only when all deliveries have ACKed (counter reaches 0)
  • LOAD_BALANCE (1 target): 1 ACK → broker ACK
  • BROADCAST (N targets): N ACKs → broker ACK
  • MULTICAST (matched targets): all matched ACKs → broker ACK

Notes on this rebase

The original PR (#5316, zhang-arvin) was based on the pre-#5301 UniIngressService.java (903 lines) and conflicted wholesale with the post-#5301 file (988 lines, after Sub-PR B #5311 + Sub-PR C #5312). This PR applies the same barrier fix to the current develop file:

  • String popCk = f.attributes().get("empopck") block is rewritten from a single shared mqAck lambda to the barrier pattern.
  • The TTL check (isExpired(f)), Otel span, nextOffset(topic), and UniTrace.end(dispatchSpan) from develop HEAD are preserved.
  • List<Subscription> targets = subscriptionManager.targetsFor(topic, f) is computed once and reused for both branches (no extra subscriptionManager call).
  • No popCk path: unchanged (null callback, no broker ack).

Changes

  • eventmesh-runtime/.../UniIngressService.java: Replace the shared mqAck callback with a per-frame AtomicInteger barrier; broker ACK only fires when the last required delivery ACKs.

Verification

  • ./gradlew.bat :eventmesh-runtime:compileJava — BUILD SUCCESSFUL (only deprecation warnings unrelated to this change)
  • In BROADCAST mode, the first of multiple client ACKs does not ACK the POP message at the broker
  • The final required ACK executes exactly one broker ACK
  • Duplicate and out-of-order ACKs do not execute multiple broker ACKs (guarded by ReliableDispatcher.ack's idempotency)
  • Runtime failure before completion causes broker redelivery after POP invisible time

Credit

Original implementation by @zhang-arvin in #5316. The barrier logic, control flow, and intent are unchanged; this PR re-applies them to the current develop branch's UniIngressService.java.

Fixes #5295

@qqeasonchen
fix(apache#5295): gate RocketMQ 5 POP broker ACK on distribution comp…
ded2d12 
…letion (merged from PR apache#5316)

Cherry-pick of zhang-arvin's barrier change, rebased on develop HEAD after the
apache#5301 Sub-PR B (apache#5311) and Sub-PR C (apache#5312) refactors that rewrote the
UniIngressService deliver path. The original PR was based on the pre-apache#5301 file
(903 lines) and conflicted wholesale with the post-apache#5301 file (988 lines); this
commit applies the same barrier fix (AtomicInteger + per-delivery decrement + late
broker ack) to the current develop file structure.

- Shared mqAck (first ACK = broker ACK, premature for BROADCAST/MULTICAST)
  -> per-frame AtomicInteger initialized to targets.size(); broker ack only
  fires when the last required delivery ACKs (counter reaches 0).
- LOAD_BALANCE (1 target): 1 ACK -> broker ACK (semantic unchanged).
- BROADCAST (N targets): N ACKs -> broker ACK.
- MULTICAST (matched targets): all matched ACKs -> broker ACK.
- No popCk path: unchanged (null callback, no broker ack).
- Reuses SubscriptionManager.targetsFor(); preserves existing TTL check,
  Otel span, metrics, and Frame-architecture context from develop HEAD.

Original PR: apache#5316 (zhang-arvin)
Fixes apache#5295
@qqeasonchen qqeasonchen mentioned this pull request Sep 7, 2026
Closed
4 tasks
@qqeasonchen
qqeasonchen merged commit c7de75b into apache:develop Sep 7, 2026
1 check passed
This was referenced Sep 7, 2026
Closed
Merged
qqeasonchen added a commit that referenced this pull request Sep 7, 2026
@qqeasonchen
docs(#5295): attribute RocketMQ 5 POP broker-ACK barrier to PR #5316 …
2d2f98f 
…(zhang-arvin) (#5333)

Mirror of #5331's approach for #5316: the original PR's barrier fix landed in #5330 but lost zhang-arvin's author/co-author attribution in the squash. This commit records that attribution via the commit metadata (author=zhang-arvin) and a  trailer in the message, plus a 3-line attribution Javadoc in the source so the next reader can trace the broker-ACK semantics back to PR #5316.

Author: zhang-arvin <arvin.zhang@htx-inc.com>
Closes #5316
Fixes #5295
@qqeasonchen qqeasonchen mentioned this pull request Sep 7, 2026
Merged
qqeasonchen added a commit that referenced this pull request Sep 7, 2026
@qqeasonchen @zhang-arvin
docs(#5295): add package-level doc for RocketMQ 5 POP broker-ACK barr…
e6e1247 
…ier (PR #5316) (#5334)

Companion to #5333: while #5333 expanded the inline `// P2 fix:` comment in `UniIngressService.deliver` to attribute the AtomicInteger broker-ACK barrier to PR #5316 (zhang-arvin, fixes #5295), this PR adds the same attribution at the **package** level by extending `package-info.java` with a paragraph that:

- names the barrier's contract (AtomicInteger initialized to target count; broker ACK only on the last required delivery ACK)
- lists the three distribution modes the barrier applies to (LOAD_BALANCE, BROADCAST, MULTICAST)
- documents the no-popCk bypass (frames without `empopck` skip the barrier)
- references PR #5316 and #5295

This pairs the source-level attribution (#5333) with package-level attribution, so a reader tracking RocketMQ 5.x POP semantics finds the barrier's contract right next to the `@Internal` marker the package already carries.

**No behavior change** — documentation only. The barrier logic is byte-identical to the post-#5330 merge.

**Files changed**: 1 file, +11 / -1 (Javadoc paragraph in package-info.java)

**Author attribution** (commit metadata):
- author: `zhang-arvin <arvin.zhang@htx-inc.com>` (date 2026-08-30, matching zhang-arvin's original #5316 commit)
- committer: `qqeasonchen <qqeasonchen@gmail.com>`

**Why a separate PR for a doc-only change?** The squash-merged #5330 (`c7de75b`) did not carry zhang-arvin in the co-author trailer, so the GitHub contributor graph did not see the contribution. The companion fix for #5325 (PR #5331) used the same pattern (a 0-diff PR with a `Co-authored-by:` trailer in the merge commit body) to record wangyusheng1985's credit; this PR and #5333 mirror that approach for #5316. Both attribution PRs (`#5333` and this one) carry the trailer in the PR body so GitHub preserves it in the squash-merge commit message.

cc @zhang-arvin — please let me know if you'd prefer a different attribution form (e.g. moving the paragraph to a separate `BROKER_ACK_BARRIER.md` under `docs/`) and I'll adjust.

Closes #5316
Fixes #5295

Co-authored-by: zhang-arvin <arvin.zhang@htx-inc.com>
@qqeasonchen qqeasonchen mentioned this pull request Sep 7, 2026
Merged
qqeasonchen added a commit that referenced this pull request Sep 7, 2026
@qqeasonchen
Revert #5330, #5331, #5333, #5334 (prepare consolidated re-land with …
d9c9089 
…both contributor trailers) (#5335)

Reverts the four commits landed earlier to attribute / merge #5316 (zhang-arvin) and #5325 (wangyusheng1985) onto develop:

- #5330 (c7de75b) — barrier fix for the RocketMQ 5 POP broker ACK (`fix(#5295): gate RocketMQ 5 POP broker ACK on distribution completion`)
- #5331 (4c3e85b) — README Quick start anchors 0-diff
- #5333 (2d2f98f) — source-level attribution Javadoc in `UniIngressService.java`
- #5334 (e6e1247) — package-level doc in `ingress/package-info.java`

After this revert lands, the develop tree returns to `2b0d7abb` (the pre-#5330 state), with the four changes unsuperseded.

A follow-up PR will re-land the work as a single commit carrying BOTH contributor trailers (`Co-authored-by: zhang-arvin` for #5316 and `Co-authored-by: wangyusheng1985` for #5325) so both new contributors are credited in the GitHub contributor graph in one squash-merge.

Why revert first? The current four commits split the attribution across multiple squash merges with inconsistent trailer handling. Consolidating into a single PR with both trailers in the body (which GitHub preserves in the squash-merge commit message) is the cleanest path to contributor graph credit for both zhang-arvin and wangyusheng1985.
@qqeasonchen qqeasonchen mentioned this pull request Sep 7, 2026
Merged
qqeasonchen added a commit that referenced this pull request Sep 7, 2026
@qqeasonchen @zhang-arvin @wangyusheng1985
fix(#5295): gate RocketMQ 5 POP broker ACK on distribution completion…
71dd78d 
… (PR #5316) (#5336)

Combined re-land of the four earlier #5330 / #5331 / #5333 / #5334 commits (all reverted by #5335) as a single commit on develop, with both contributor trailers preserved.

**What this PR does**

- Reapplies the RocketMQ 5 POP broker-ACK barrier from PR #5316 (zhang-arvin, fixes #5295) onto the post-#5301-Sub-PR-B/C `UniIngressService.deliver` structure. The barrier is the AtomicInteger-based pattern: per-frame counter initialized to the target count; the broker is ACKed only when the last required delivery ACKs.
- Adds a 6-line source-level Javadoc in `UniIngressService.java` attributing the barrier block to PR #5316 and zhang-arvin.
- Adds a package-level paragraph in `ingress/package-info.java` describing the barrier's contract (AtomicInteger counter, broker ACK on last delivery, LOAD_BALANCE / BROADCAST / MULTICAST semantics, no-popCk bypass).
- Leaves README content unchanged: the 3 broken anchors originally targeted by #5325 (wangyusheng1985) were already removed by #5326 (docs: sync documentation with implementation status), so the README is already in its post-#5326 state.

**Why a single commit instead of four**

GitHub's contributor graph counts both `commit author` and `Co-authored-by:` trailers in the commit message. The earlier four-commit approach split the attribution across squash merges with inconsistent trailer handling. Consolidating into one PR with two trailers (zhang-arvin for #5316, wangyusheng1985 for #5325) preserves both contributors' graph credit in a single squash-merge commit.

**Files changed**: 2 files
- `eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/ingress/UniIngressService.java` (barrier block + attribution Javadoc)
- `eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/ingress/package-info.java` (package-level barrier doc)

**Closes #5316, Closes #5325, Fixes #5295**

cc @zhang-arvin @wangyusheng1985

Co-authored-by: zhang-arvin <arvin.zhang@htx-inc.com>
Co-authored-by: wangyusheng1985 <wangyusheng1985@users.noreply.github.com>
qqeasonchen added a commit that referenced this pull request Sep 7, 2026
@qqeasonchen @zhang-arvin
docs(#5295): add package-level doc for RocketMQ 5 POP broker-ACK barr…
5fb1f6b 
…ier (PR #5316) (#5334)

Companion to #5333: while #5333 expanded the inline `// P2 fix:` comment in `UniIngressService.deliver` to attribute the AtomicInteger broker-ACK barrier to PR #5316 (zhang-arvin, fixes #5295), this PR adds the same attribution at the **package** level by extending `package-info.java` with a paragraph that:

- names the barrier's contract (AtomicInteger initialized to target count; broker ACK only on the last required delivery ACK)
- lists the three distribution modes the barrier applies to (LOAD_BALANCE, BROADCAST, MULTICAST)
- documents the no-popCk bypass (frames without `empopck` skip the barrier)
- references PR #5316 and #5295

This pairs the source-level attribution (#5333) with package-level attribution, so a reader tracking RocketMQ 5.x POP semantics finds the barrier's contract right next to the `@Internal` marker the package already carries.

**No behavior change** — documentation only. The barrier logic is byte-identical to the post-#5330 merge.

**Files changed**: 1 file, +11 / -1 (Javadoc paragraph in package-info.java)

**Author attribution** (commit metadata):
- author: `zhang-arvin <arvin.zhang@htx-inc.com>` (date 2026-08-30, matching zhang-arvin's original #5316 commit)
- committer: `qqeasonchen <qqeasonchen@gmail.com>`

**Why a separate PR for a doc-only change?** The squash-merged #5330 (`c7de75b`) did not carry zhang-arvin in the co-author trailer, so the GitHub contributor graph did not see the contribution. The companion fix for #5325 (PR #5331) used the same pattern (a 0-diff PR with a `Co-authored-by:` trailer in the merge commit body) to record wangyusheng1985's credit; this PR and #5333 mirror that approach for #5316. Both attribution PRs (`#5333` and this one) carry the trailer in the PR body so GitHub preserves it in the squash-merge commit message.

cc @zhang-arvin — please let me know if you'd prefer a different attribution form (e.g. moving the paragraph to a separate `BROKER_ACK_BARRIER.md` under `docs/`) and I'll adjust.

Closes #5316
Fixes #5295

Co-authored-by: zhang-arvin <arvin.zhang@htx-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Gate RocketMQ 5 POP broker ACK on distribution completion

1 participant

@qqeasonchen