Skip to content

[CELEBORN-2428] Fix TierWriter buffer accounting when CompositeByteBuf insertion fails - #3809

Open
Kalvin2077 wants to merge 1 commit into
apache:mainfrom
Kalvin2077:fix/CELEBORN-2428
Open

[CELEBORN-2428] Fix TierWriter buffer accounting when CompositeByteBuf insertion fails#3809
Kalvin2077 wants to merge 1 commit into
apache:mainfrom
Kalvin2077:fix/CELEBORN-2428

Conversation

@Kalvin2077

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Fix TierWriter buffer accounting and reference-count handling when CompositeByteBuf.addComponent fails before insertion or during consolidation. Add regression tests and include the shuffle key in write-error logs.

Why are the changes needed?

addComponent can fail before or after taking ownership of the buffer. Treating both cases identically can cause incorrect storage counters or buffer reference-count leaks.

Does this PR resolve a correctness bug?

  • Yes

Does this PR introduce any user-facing change?

  • Yes

How was this patch tested?

  • Added tests for pre-insertion OOM, capacity overflow, and post-insertion consolidation OOM.
  • Ran TierWriterSuite: 11 tests passed.
  • Ran ./build/mvn spotless:check -pl worker.

@Kalvin2077 Kalvin2077 changed the title [CELEBORN-2428]: Fix TierWriter buffer accounting when CompositeByteBuf insertion fails [CELEBORN-2428] Fix TierWriter buffer accounting when CompositeByteBuf insertion fails Aug 19, 2026
@Kalvin2077

Copy link
Copy Markdown
Contributor Author

@zaynt4606 @RexXiong @SteNicholas
PING

@github-actions github-actions Bot added module:worker correctness Correctness bugfix labels Aug 19, 2026
@yew1eb

yew1eb commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@Kalvin2077 Overall LGTM. Minor note: in Netty 4.2.10+ addComponent0 already does if (!added) buf.release() on pre-insertion failure, so the main issue here is the storeage-counter inflation rather than a ref-count leak.
By the way, the correctness label should be removed.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants