Skip to content

Conversation

@cchung100m
Copy link
Collaborator

What changes were proposed in this pull request?

Fix IllegalReferenceCountException about ShuffleBlockInfo

Why are the changes needed?

for #2517

Does this PR introduce any user-facing change?

No.

How was this patch tested?

current UT

@cchung100m cchung100m self-assigned this Oct 9, 2025
@github-actions
Copy link

github-actions bot commented Oct 9, 2025

Test Results

 3 130 files  ±0   3 130 suites  ±0   6h 51m 27s ⏱️ +16s
 1 206 tests ±0   1 205 ✅ ±0   1 💤 ±0  0 ❌ ±0 
15 274 runs  ±0  15 259 ✅ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit 93eba64. ± Comparison against base commit 4805d13.

♻️ This comment has been updated with latest results.

@cchung100m cchung100m marked this pull request as ready for review October 10, 2025 04:12
@cchung100m cchung100m requested a review from zuston October 10, 2025 04:13
List<ShuffleBlock> shuffleBlocks = Lists.newArrayList();
int partitionRequireSize = 0;
for (ShuffleBlockInfo sbi : ptb.getValue()) {
if (sbi.getData().refCnt() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This may not safe, I think the fast fail will be better than slient sucess.
But for this case, I haven't found the root cause about this.

Copy link
Collaborator Author

@cchung100m cchung100m Oct 19, 2025

Choose a reason for hiding this comment

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

Hi @zuston

Thanks for the reply. I updated the part you mentioned.

Since we throw the proper RssException here, we might leverage the retry machanism which introduced by #2608 to solve the data race?

Copy link
Member

@zuston zuston Oct 20, 2025

Choose a reason for hiding this comment

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

yes, this PR's change looks good to me. I will take a deep look #2608 assp in this week

List<ShuffleBlock> shuffleBlocks = Lists.newArrayList();
int partitionRequireSize = 0;
for (ShuffleBlockInfo sbi : ptb.getValue()) {
if (sbi.getData().refCnt() == 0) {
Copy link
Member

@zuston zuston Oct 20, 2025

Choose a reason for hiding this comment

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

yes, this PR's change looks good to me. I will take a deep look #2608 assp in this week

@zuston zuston changed the title [#2517] Fix(client): IllegalReferenceCountException about ShuffleBlockInfo [#2517] fix(client): IllegalReferenceCountException about ShuffleBlockInfo Oct 20, 2025
@zuston zuston merged commit 5edf952 into apache:master Oct 20, 2025
41 checks passed
@cchung100m cchung100m deleted the issue-2517 branch October 21, 2025 02:25
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.

2 participants