You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "jerqi (via GitHub)" <gi...@apache.org> on 2023/03/11 08:34:37 UTC
[GitHub] [incubator-uniffle] jerqi opened a new pull request, #712: [#133] feat: Add Netty port information for Shuffle Server
jerqi opened a new pull request, #712:
URL: https://github.com/apache/incubator-uniffle/pull/712
### What changes were proposed in this pull request?
Add Netty port information for Shuffle Server
### Why are the changes needed?
Let us can use old servers with Grpc and new servers with Netty at the same time.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Add new ut
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #712: [#711] feat: Add Netty port information for Shuffle Server
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1464890971
## [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#712](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (17aa9af) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/5882e10122ffd9b2d1bfb39630f32c643cf7ef0d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5882e10) will **increase** coverage by `1.10%`.
> The diff coverage is `53.96%`.
```diff
@@ Coverage Diff @@
## master #712 +/- ##
============================================
+ Coverage 60.63% 61.74% +1.10%
+ Complexity 1845 1754 -91
============================================
Files 228 205 -23
Lines 12702 10199 -2503
Branches 1062 1020 -42
============================================
- Hits 7702 6297 -1405
+ Misses 4592 3566 -1026
+ Partials 408 336 -72
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/hadoop/mapred/RssMapOutputCollector.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1Jzc01hcE91dHB1dENvbGxlY3Rvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SVXRpbHMuamF2YQ==) | `48.24% <0.00%> (-2.22%)` | :arrow_down: |
| [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3YyL2FwcC9Sc3NNUkFwcE1hc3Rlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...he/uniffle/coordinator/CoordinatorGrpcService.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JHcnBjU2VydmljZS5qYXZh) | `2.06% <0.00%> (-0.02%)` | :arrow_down: |
| [...ache/uniffle/coordinator/SimpleClusterManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvU2ltcGxlQ2x1c3Rlck1hbmFnZXIuamF2YQ==) | `75.31% <0.00%> (ø)` | |
| [...a/org/apache/uniffle/server/RegisterHeartBeat.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9SZWdpc3RlckhlYXJ0QmVhdC5qYXZh) | `42.37% <0.00%> (-0.74%)` | :arrow_down: |
| [.../java/org/apache/uniffle/server/ShuffleServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyLmphdmE=) | `62.80% <42.85%> (-0.75%)` | :arrow_down: |
| [...a/org/apache/uniffle/common/ShuffleServerInfo.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9TaHVmZmxlU2VydmVySW5mby5qYXZh) | `80.64% <87.50%> (+5.64%)` | :arrow_up: |
| [...ava/org/apache/uniffle/coordinator/ServerNode.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvU2VydmVyTm9kZS5qYXZh) | `83.33% <90.00%> (+2.08%)` | :arrow_up: |
| [...ain/java/org/apache/uniffle/common/ClientType.java](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9DbGllbnRUeXBlLmphdmE=) | `100.00% <100.00%> (ø)` | |
| ... and [1 more](https://codecov.io/gh/apache/incubator-uniffle/pull/712?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [24 files with indirect coverage changes](https://codecov.io/gh/apache/incubator-uniffle/pull/712/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134896420
##########
common/src/main/java/org/apache/uniffle/common/ClientType.java:
##########
@@ -18,5 +18,6 @@
package org.apache.uniffle.common;
public enum ClientType {
- GRPC
+ GRPC,
+ GRPC_NETTY
Review Comment:
Oh, I see. I thought the client is used to push data via netty directly and only. Instead the client will request both data transfer and other rpc requests. Then GRPC_NETTY is more appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465673056
> > I'm not sure whether we should introduce a new port here.. It adds some deployment overhead. Is there ways to multiplex port here?
>
> It's too complex to support that one port can use multiple protocols.
OK. Then is it still required to use a fixed port for Netty service? I'd like the netty service could be bind for random available port.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465678399
> > > I'm not sure whether we should introduce a new port here.. It adds some deployment overhead. Is there ways to multiplex port here?
> >
> >
> > It's too complex to support that one port can use multiple protocols.
>
> OK. Then is it still required to use a fixed port for Netty service? I'd like the netty service could be bind for random available port.
That's another pr. If we support to bind random port, we should support both Grpc and Netty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1133188852
##########
client-mr/src/main/java/org/apache/hadoop/mapred/RssMapOutputCollector.java:
##########
@@ -143,20 +143,14 @@ private Map<Integer, List<ShuffleServerInfo>> createAssignmentMap(JobConf jobCon
}
String[] splitServers = servers.split(",");
List<ShuffleServerInfo> assignServers = Lists.newArrayList();
- for (String splitServer : splitServers) {
- String[] serverInfo = splitServer.split(":");
- if (serverInfo.length != 2) {
- throw new RssException("partition " + i + " server info isn't right");
- }
- ShuffleServerInfo sever = new ShuffleServerInfo(StringUtils.join(serverInfo, "-"),
- serverInfo[0], Integer.parseInt(serverInfo[1]));
- assignServers.add(sever);
- }
+ RssMRUtils.buildAssignServers(i, splitServers, assignServers);
partitionToServers.put(i, assignServers);
}
return partitionToServers;
}
+
Review Comment:
Remove blank line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465578016
> I'm not sure whether we should introduce a new port here.. It adds some deployment overhead. Is there ways to multiplex port here?
It's too complex to support that one port can use multiple protocols.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465696120
> > > > > I'm not sure whether we should introduce a new port here.. It adds some deployment overhead. Is there ways to multiplex port here?
> > > >
> > > >
> > > > It's too complex to support that one port can use multiple protocols.
> > >
> > >
> > > OK. Then is it still required to use a fixed port for Netty service? I'd like the netty service could be bind for random available port.
> >
> >
> > That's another pr. If we support to bind random port, we should support both Grpc and Netty.
>
> Yeah. That's another PR, but we will support random port, right? Then, I'd like to propose some changes in this PR.
What are changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134829296
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -397,6 +397,13 @@ public class ShuffleServerConf extends RssBaseConf {
.defaultValue(true)
.withDescription("Whether shutdown the server after server is decommissioned");
+ public static final ConfigOption<Integer> NETTY_SERVER_PORT = ConfigOptions
+ .key("rss.server.netty.port")
+ .intType()
+ .checkValue(ConfigUtils.POSITIVE_INTEGER_VALIDATOR_2, "netty port must be positive")
+ .defaultValue(0)
Review Comment:
ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134829073
##########
common/src/main/java/org/apache/uniffle/common/ShuffleServerInfo.java:
##########
@@ -25,19 +25,28 @@ public class ShuffleServerInfo implements Serializable {
private String host;
- private int port;
+ private int grpcPort;
+
+ private int nettyPort = 0;
Review Comment:
ok.
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -40,6 +40,7 @@ public class ServerNode implements Comparable<ServerNode> {
private boolean isHealthy;
private final ServerStatus status;
private Map<String, StorageInfo> storageInfo;
+ private int nettyPort = 0;
Review Comment:
ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134824155
##########
common/src/main/java/org/apache/uniffle/common/ClientType.java:
##########
@@ -18,5 +18,6 @@
package org.apache.uniffle.common;
public enum ClientType {
- GRPC
+ GRPC,
+ GRPC_NETTY
Review Comment:
I think current implementation is a mixed way, only main data transfer api will be changed to netty proto. So GRPC_NETTY is reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465691257
> > > > I'm not sure whether we should introduce a new port here.. It adds some deployment overhead. Is there ways to multiplex port here?
> > >
> > >
> > > It's too complex to support that one port can use multiple protocols.
> >
> >
> > OK. Then is it still required to use a fixed port for Netty service? I'd like the netty service could be bind for random available port.
>
> That's another pr. If we support to bind random port, we should support both Grpc and Netty.
Yeah. That's another PR, but we will support random port, right? Then, I'd like to propose some changes in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] leixm commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465219176
+1 LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#issuecomment-1465571376
I'm not sure whether we should introduce a new port here.. It adds some deployment overhead.
Is there ways to multiplex port here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134817772
##########
common/src/main/java/org/apache/uniffle/common/ShuffleServerInfo.java:
##########
@@ -25,19 +25,28 @@ public class ShuffleServerInfo implements Serializable {
private String host;
- private int port;
+ private int grpcPort;
+
+ private int nettyPort = 0;
Review Comment:
If we are going to support random port. I believe port=`0` has special meanings.
How about make the default value of nettyPort to be -1?
##########
common/src/main/java/org/apache/uniffle/common/ClientType.java:
##########
@@ -18,5 +18,6 @@
package org.apache.uniffle.common;
public enum ClientType {
- GRPC
+ GRPC,
+ GRPC_NETTY
Review Comment:
is `NETTY` more appropriate?
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -40,6 +40,7 @@ public class ServerNode implements Comparable<ServerNode> {
private boolean isHealthy;
private final ServerStatus status;
private Map<String, StorageInfo> storageInfo;
+ private int nettyPort = 0;
Review Comment:
ditto.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -182,8 +183,13 @@ private void initialization() throws Exception {
if (ip == null) {
throw new RuntimeException("Couldn't acquire host Ip");
}
- port = shuffleServerConf.getInteger(ShuffleServerConf.RPC_SERVER_PORT);
- id = ip + "-" + port;
+ grpcPort = shuffleServerConf.getInteger(ShuffleServerConf.RPC_SERVER_PORT);
+ nettyPort = shuffleServerConf.getInteger(ShuffleServerConf.NETTY_SERVER_PORT);
+ if (nettyPort > 0) {
+ id = ip + "-" + grpcPort + "-" + nettyPort;
+ } else {
+ id = ip + "-" + grpcPort;
Review Comment:
The id should be the actual port if passed port is zero for random port binding.
But this could be addressed in another PR.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -397,6 +397,13 @@ public class ShuffleServerConf extends RssBaseConf {
.defaultValue(true)
.withDescription("Whether shutdown the server after server is decommissioned");
+ public static final ConfigOption<Integer> NETTY_SERVER_PORT = ConfigOptions
+ .key("rss.server.netty.port")
+ .intType()
+ .checkValue(ConfigUtils.POSITIVE_INTEGER_VALIDATOR_2, "netty port must be positive")
+ .defaultValue(0)
Review Comment:
how about make the default value -1? And reserve `0` for later random binding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134828472
##########
common/src/main/java/org/apache/uniffle/common/ClientType.java:
##########
@@ -18,5 +18,6 @@
package org.apache.uniffle.common;
public enum ClientType {
- GRPC
+ GRPC,
+ GRPC_NETTY
Review Comment:
> I think current implementation is a mixed way, only main data transfer api will be changed to netty proto. So GRPC_NETTY is reasonable.
+ 1.
##########
common/src/main/java/org/apache/uniffle/common/ClientType.java:
##########
@@ -18,5 +18,6 @@
package org.apache.uniffle.common;
public enum ClientType {
- GRPC
+ GRPC,
+ GRPC_NETTY
Review Comment:
> I think current implementation is a mixed way, only main data transfer api will be changed to netty proto. So GRPC_NETTY is reasonable.
+1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org