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

[#627] fix(operator): support specifying custom ports #629

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

wangao1236
Copy link
Collaborator

@wangao1236 wangao1236 commented Feb 17, 2023

What changes were proposed in this pull request?

Set coordinator/shuffler server's container port to the fields of RSS spec

Why are the changes needed?

Fix #627.

Does this PR introduce any user-facing change?

For RSS cluster admin, they can set custom ports for shuffle servers and coordinators.

How was this patch tested?

Manually verified.

@advancedxy advancedxy changed the title [operator]: support specifying custom ports [#627] fix(operator): support specifying custom ports Feb 17, 2023
Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

LGTM, except one minor comment

advancedxy
advancedxy previously approved these changes Feb 20, 2023
@jerqi
Copy link
Contributor

jerqi commented Feb 20, 2023

Should this pr be included in 0.7? @advancedxy

@advancedxy
Copy link
Contributor

Should this pr be included in 0.7? @advancedxy

It's better to include this one since it's a bug fix. also cc @zuston

@zuston
Copy link
Member

zuston commented Feb 20, 2023

Should this pr be included in 0.7? @advancedxy

It's better to include this one since it's a bug fix. also cc @zuston

It's OK for me.

@advancedxy
Copy link
Contributor

Thanks @wangao1236, merging this. And if possible, would you cherry-pick this pr to branch-0.7?

@advancedxy advancedxy merged commit b31a5c9 into apache:master Feb 20, 2023
@jerqi
Copy link
Contributor

jerqi commented Feb 20, 2023

Thanks @wangao1236, merging this. And if possible, would you cherry-pick this pr to branch-0.7?

If there are no conficts, you can commit directly.

jerqi pushed a commit that referenced this pull request Feb 21, 2023
### What changes were proposed in this pull request?
Set coordinator/shuffler server's container port to the fields of RSS spec

### Why are the changes needed?
Fix #627.

### Does this PR introduce _any_ user-facing change?
For RSS cluster admin, they can set custom ports for shuffle servers and coordinators.

### How was this patch tested?
Manually verified.
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] operator should set coordinator and shuffle server's ports to the spec rather a fixed value
4 participants