-
Notifications
You must be signed in to change notification settings - Fork 355
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-1513] Support wildcard bind in dual stack environments #2713
Conversation
this is a duplicate of #2633 -- I had forgotten to update it for a long time and it became stale with a lot of conflicts, hence I just made a new branch/PR. |
cc @FMX @SteNicholas @pan3793 @RexXiong @turboFei please review, thank you :) |
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
Outdated
Show resolved
Hide resolved
thanks @RexXiong , I have addressed your comments. Please take another look , thanks!! :) |
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@RexXiong @SteNicholas are we able to merge this? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Merged into main(0.6.0).
thanks everyone for the reviews!! |
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java
Show resolved
Hide resolved
### What changes were proposed in this pull request? enrich the docs for supporting wildcard address bind in this [PR](#2713). ### Why are the changes needed? better docs ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A - just docs change Closes #2736 from akpatnam25/CELEBORN-1513-doc-followup. Authored-by: Aravind Patnam <[email protected]> Signed-off-by: Cheng Pan <[email protected]>
### What changes were proposed in this pull request? Support wildcard bind for RPC and HTTP servers. When wildcard address is used, the service is able to listen to both ipv4 and ipv6 traffic in dual-stack environments. The specific scenario where this becomes relevant is as follows: If some of the compute infrastructure is IPv4 only, some v6 only and others dual stack - the way we can have a single Celeborn infra to cater to all is by: a) Set bind.preferip to false - so that advertised address is the host and not IP. b) bind to wild card address With both in place, the v4 only compute nodes will resolve the v4 address and connect to v4 ip/port. Likewise, for v6 only. Dual stack compute nodes will use prefer ipv6 Java flag to resolve to either v4 or v6. This is how we are handling the combination of scenarios internally. ### Why are the changes needed? see above. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Tested on a server using netstat, and tried connecting to via `nc -4` and `nc -6` to ensure connection was there. Closes apache#2713 from akpatnam25/CELEBORN-1513-fix. Authored-by: Aravind Patnam <[email protected]> Signed-off-by: mingji <[email protected]>
### What changes were proposed in this pull request? enrich the docs for supporting wildcard address bind in this [PR](apache#2713). ### Why are the changes needed? better docs ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A - just docs change Closes apache#2736 from akpatnam25/CELEBORN-1513-doc-followup. Authored-by: Aravind Patnam <[email protected]> Signed-off-by: Cheng Pan <[email protected]>
…e log in startup document ### What changes were proposed in this pull request? Update `advertised endpoint` of master service log in startup document. ### Why are the changes needed? #2713 has changed the startup log of of master service in `NettyRpcEnv`, which should update the log of startup document. ``` logInfo(s"Starting RPC Server [${config.name}] on ${config.bindAddress}:$actualPort " + s"with advertised endpoint ${config.advertiseAddress}:$actualPort") ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes #2773 from SteNicholas/CELEBORN-1513. Authored-by: SteNicholas <[email protected]> Signed-off-by: mingji <[email protected]>
…e log in startup document ### What changes were proposed in this pull request? Update `advertised endpoint` of master service log in startup document. ### Why are the changes needed? apache#2713 has changed the startup log of of master service in `NettyRpcEnv`, which should update the log of startup document. ``` logInfo(s"Starting RPC Server [${config.name}] on ${config.bindAddress}:$actualPort " + s"with advertised endpoint ${config.advertiseAddress}:$actualPort") ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes apache#2773 from SteNicholas/CELEBORN-1513. Authored-by: SteNicholas <[email protected]> Signed-off-by: mingji <[email protected]>
### What changes were proposed in this pull request? Support wildcard bind for RPC and HTTP servers. When wildcard address is used, the service is able to listen to both ipv4 and ipv6 traffic in dual-stack environments. The specific scenario where this becomes relevant is as follows: If some of the compute infrastructure is IPv4 only, some v6 only and others dual stack - the way we can have a single Celeborn infra to cater to all is by: a) Set bind.preferip to false - so that advertised address is the host and not IP. b) bind to wild card address With both in place, the v4 only compute nodes will resolve the v4 address and connect to v4 ip/port. Likewise, for v6 only. Dual stack compute nodes will use prefer ipv6 Java flag to resolve to either v4 or v6. This is how we are handling the combination of scenarios internally. ### Why are the changes needed? see above. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Tested on a server using netstat, and tried connecting to via `nc -4` and `nc -6` to ensure connection was there. Closes apache#2713 from akpatnam25/CELEBORN-1513-fix. Authored-by: Aravind Patnam <[email protected]> Signed-off-by: mingji <[email protected]>
### What changes were proposed in this pull request? enrich the docs for supporting wildcard address bind in this [PR](apache#2713). ### Why are the changes needed? better docs ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A - just docs change Closes apache#2736 from akpatnam25/CELEBORN-1513-doc-followup. Authored-by: Aravind Patnam <[email protected]> Signed-off-by: Cheng Pan <[email protected]>
…e log in startup document ### What changes were proposed in this pull request? Update `advertised endpoint` of master service log in startup document. ### Why are the changes needed? apache#2713 has changed the startup log of of master service in `NettyRpcEnv`, which should update the log of startup document. ``` logInfo(s"Starting RPC Server [${config.name}] on ${config.bindAddress}:$actualPort " + s"with advertised endpoint ${config.advertiseAddress}:$actualPort") ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes apache#2773 from SteNicholas/CELEBORN-1513. Authored-by: SteNicholas <[email protected]> Signed-off-by: mingji <[email protected]>
What changes were proposed in this pull request?
Support wildcard bind for RPC and HTTP servers. When wildcard address is used, the service is able to listen to both ipv4 and ipv6 traffic in dual-stack environments.
The specific scenario where this becomes relevant is as follows:
If some of the compute infrastructure is IPv4 only, some v6 only and others dual stack - the way we can have a single Celeborn infra to cater to all is by:
a) Set bind.preferip to false - so that advertised address is the host and not IP.
b) bind to wild card address
With both in place, the v4 only compute nodes will resolve the v4 address and connect to v4 ip/port.
Likewise, for v6 only.
Dual stack compute nodes will use prefer ipv6 Java flag to resolve to either v4 or v6.
This is how we are handling the combination of scenarios internally.
Why are the changes needed?
see above.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Tested on a server using netstat, and tried connecting to via
nc -4
andnc -6
to ensure connection was there.