Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2282] improvement(spark3): Invoke with object self in DelegationRssShuffleManager #2283

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

zuston
Copy link
Member

@zuston zuston commented Dec 10, 2024

What changes were proposed in this pull request?

Fix the bug of none object self when invoking the reflection method.

Why are the changes needed?

Fix: #2282

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests

@zuston zuston requested a review from jerqi December 10, 2024 02:44
@jerqi
Copy link
Contributor

jerqi commented Dec 10, 2024

What will it happen if we don't have this?

Copy link

Test Results

 2 966 files  +23   2 966 suites  +23   6h 32m 32s ⏱️ + 13m 34s
 1 096 tests ± 0   1 094 ✅ + 1   2 💤 ±0  0 ❌  - 1 
13 735 runs  +53  13 705 ✅ +54  30 💤 ±0  0 ❌  - 1 

Results for commit 4afa783. ± Comparison against base commit 0552d3b.

@zuston
Copy link
Member Author

zuston commented Dec 10, 2024

What will it happen if we don't have this?

Nothing will happen, because we bypass the getReader in the DelegationRssShuffleManager, so the acutall getReader invoking will happen in the underlying concrate shuffle manager. But this is an obvious bug, if someone wants to implement custom shuffle manager, he will follow the incorrect examples.

@jerqi
Copy link
Contributor

jerqi commented Dec 10, 2024

What will it happen if we don't have this?

Nothing will happen, because we bypass the getReader in the DelegationRssShuffleManager, so the acutall getReader invoking will happen in the underlying concrate shuffle manager. But this is an obvious bug, if someone wants to implement custom shuffle manager, he will follow the incorrect examples.

So I think this is an improvement instead of a bug fix. If this is bug fix, we would better have a ut for it.

@jerqi jerqi changed the title [#2282] fix(spark3): Invoke with object self in DelegationRssShuffleManager [#2282] improvement(spark3): Invoke with object self in DelegationRssShuffleManager Dec 10, 2024
Copy link
Member

@maobaolong maobaolong left a comment

Choose a reason for hiding this comment

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

@zuston Good catch! We encountered this issue too in this week! Thanks for your contribution!

@maobaolong maobaolong merged commit b7c9ca5 into apache:master Dec 11, 2024
43 checks passed
@wForget
Copy link
Member

wForget commented Dec 11, 2024

Nothing will happen, because we bypass the getReader in the DelegationRssShuffleManager, so the acutall getReader invoking will happen in the underlying concrate shuffle manager. But this is an obvious bug, if someone wants to implement custom shuffle manager, he will follow the incorrect examples.

I prefer to infer the appropriate shuffle manager from ShuffleHandle rather than overriding the shuffle manager configuration on the dirver side, so that we can implement stage-level fallback.

jerqi pushed a commit that referenced this pull request Jan 1, 2025
…anager (#2283)

Co-authored-by: Junfan Zhang <[email protected]>

### What changes were proposed in this pull request?

Fix the bug of none object self when invoking the reflection method.

### Why are the changes needed?

Fix: #2282

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing tests
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] Invalid invoking in the DelegationRssShuffleManager#getReader
4 participants