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

[CELEBORN-1811] Update default value for celeborn.master.slot.assign.extraSlots #3039

Closed
wants to merge 3 commits into from

Conversation

FMX
Copy link
Contributor

@FMX FMX commented Dec 30, 2024

What changes were proposed in this pull request?

To avoid possible worker load skew for the stages with tiny reducer numbers.

Why are the changes needed?

If a stage has tiny reducers and skewed partitions, The default value will lead to serious worker load imbalance cause some workers unable to handle shuffle data.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

GA and cluster test.

@SteNicholas
Copy link
Member

Please update migration.md to describe the change for the default value.

@FMX
Copy link
Contributor Author

FMX commented Dec 31, 2024

@pan3793 @SteNicholas This PR is ready. How about now?

@pan3793 pan3793 changed the title [CELEBORN-1811] Update default value for celeborn.master.slot.assignextraSlots [CELEBORN-1811] Update default value for celeborn.master.slot.assign.extraSlots Dec 31, 2024
@pan3793
Copy link
Member

pan3793 commented Dec 31, 2024

Please update migration.md to describe the change for the default value.

@FMX same request

@FMX
Copy link
Contributor Author

FMX commented Dec 31, 2024

The migration guide has been updated.

@SteNicholas
Copy link
Member

SteNicholas commented Dec 31, 2024

LGTM. Merged to main(v0.6.0).

SteNicholas added a commit that referenced this pull request Jan 2, 2025
…o explain default behavior in description

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

Change `celeborn.<module>.io.mode` optional to explain default behavior in description.

### Why are the changes needed?

The default value of `celeborn.<module>.io.mode` in document could be changed by whether epoll mode is available for different os. Therefore, `celeborn.<module>.io.mode` should be changed to optional and explained the default behavior in description of option.

Follow up #3039 (comment).

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

`celeborn.<module>.io.mode` is optional and explains default behavior in description.

### How was this patch tested?

CI.

Closes #3044 from SteNicholas/CELEBORN-1774.

Authored-by: SteNicholas <[email protected]>
Signed-off-by: SteNicholas <[email protected]>
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.

3 participants