You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "smallzhongfeng (via GitHub)" <gi...@apache.org> on 2023/03/18 08:44:05 UTC
[GitHub] [incubator-uniffle] smallzhongfeng opened a new pull request, #739: [#719] feat(netty): Optimize allocation strategy
smallzhongfeng opened a new pull request, #739:
URL: https://github.com/apache/incubator-uniffle/pull/739
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
Examples:
- "[#123] feat(operator): support xxx"
- "[#233] fix: check null before access result in xxx"
- "[MINOR] refactor: fix typo in variable name"
- "[MINOR] docs: fix typo in README"
- "[#255] test: fix flaky test NameOfTheTest"
Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. Contributor guidelines:
https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md
3. If the PR is unfinished, please mark this PR as draft.
-->
### What changes were proposed in this pull request?
Users can choose to use netty's transmission method or grpc's through client configuration.
### Why are the changes needed?
Fix: #719
### Does this PR introduce _any_ user-facing change?
No. However, if users want to use `netty` as a data transfer method, they need to enable `spark.rss.client.type=GRPC_ NETTY` or `mapreduce.rss.client.type=GRPC_ NETTY`
### How was this patch tested?
New uts.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#issuecomment-1474777470
## [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/739?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 [#739](https://codecov.io/gh/apache/incubator-uniffle/pull/739?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89b05a3) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/d82e10d9bf1457bb74a4ac349a117f44fcbeb31a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d82e10d) will **increase** coverage by `2.15%`.
> The diff coverage is `62.50%`.
```diff
@@ Coverage Diff @@
## master #739 +/- ##
============================================
+ Coverage 60.73% 62.88% +2.15%
- Complexity 1859 1860 +1
============================================
Files 231 217 -14
Lines 12857 10900 -1957
Branches 1071 1072 +1
============================================
- Hits 7809 6855 -954
+ Misses 4633 3689 -944
+ Partials 415 356 -59
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/739?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%> (ø)` | |
| [...a/org/apache/uniffle/server/ShuffleServerConf.java](https://codecov.io/gh/apache/incubator-uniffle/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyQ29uZi5qYXZh) | `99.45% <ø> (-0.01%)` | :arrow_down: |
| [.../java/org/apache/uniffle/server/ShuffleServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/739?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=) | `63.47% <100.00%> (-0.55%)` | :arrow_down: |
... and [14 files with indirect coverage changes](https://codecov.io/gh/apache/incubator-uniffle/pull/739/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] jerqi commented on a diff in pull request #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141749311
##########
client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java:
##########
@@ -122,4 +126,11 @@ public static void validateTestModeConf(boolean testMode, String storageType) {
+ "because of the poor performance of these two types.");
}
}
+
+ public static void validateClientType(String clientType) {
Review Comment:
I got it. My mistake.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141727950
##########
client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java:
##########
@@ -122,4 +126,11 @@ public static void validateTestModeConf(boolean testMode, String storageType) {
+ "because of the poor performance of these two types.");
}
}
+
+ public static void validateClientType(String clientType) {
Review Comment:
OK, i will try it.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141727353
##########
common/src/main/java/org/apache/uniffle/common/config/RssConf.java:
##########
@@ -645,4 +646,8 @@ public String getEnv(String key) {
return System.getenv(key);
}
+ @VisibleForTesting
+ public Map<String, Object> getSettings() {
Review Comment:
It is used in `getShuffleAssignmentsTest`.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141008419
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -248,9 +249,18 @@ private void initServerTags() {
if (CollectionUtils.isNotEmpty(configuredTags)) {
tags.addAll(configuredTags);
}
+ isNettyServerEnabled();
LOG.info("Server tags: {}", tags);
}
+ private void isNettyServerEnabled() {
Review Comment:
`tagServer` seems more 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] smallzhongfeng commented on a diff in pull request #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141032261
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -400,7 +400,6 @@ public class ShuffleServerConf extends RssBaseConf {
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")
Review Comment:
I am also wondering if it is possible to reserve a negative number of ports, which means shutting down the Netty service.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141033563
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +122,29 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ ShuffleServerConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ shuffleServerConf.setInteger("rss.rpc.server.port", SHUFFLE_SERVER_PORT + 3);
+ shuffleServerConf.setInteger("rss.jetty.http.port", 18089);
+ shuffleServerConf.setInteger(ShuffleServerConf.NETTY_SERVER_PORT, -3);
Review Comment:
I think you can get the `RssConf#settings` by reflection and remove the config options which you don't need.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141731590
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +124,30 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ RssConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ Map<String, Object> originConf = shuffleServerConf.getSettings();
+ Class<RssConf> clazz = RssConf.class;
+ Field field = clazz.getDeclaredField("settings");
+ field.setAccessible(true);
+ originConf.remove(ShuffleServerConf.NETTY_SERVER_PORT.key());
+ field.set(shuffleServerConf, originConf);
Review Comment:
We don't set the value again.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141032319
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -248,9 +249,18 @@ private void initServerTags() {
if (CollectionUtils.isNotEmpty(configuredTags)) {
tags.addAll(configuredTags);
}
+ isNettyServerEnabled();
LOG.info("Server tags: {}", tags);
}
+ private void isNettyServerEnabled() {
Review Comment:
Sounds good.
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -346,6 +347,11 @@ public <K, V, C> ShuffleHandle registerShuffle(int shuffleId, ShuffleDependency<
long retryInterval = sparkConf.get(RssSparkConfig.RSS_CLIENT_ASSIGNMENT_RETRY_INTERVAL);
int retryTimes = sparkConf.get(RssSparkConfig.RSS_CLIENT_ASSIGNMENT_RETRY_TIMES);
int estimateTaskConcurrency = RssSparkShuffleUtils.estimateTaskConcurrency(sparkConf);
+ if (sparkConf.get(RssSparkConfig.RSS_CLIENT_TYPE).equals(ClientType.GRPC_NETTY.name())) {
Review Comment:
Updated.
--
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 pull request #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#issuecomment-1476331577
Thanks for your review! @jerqi @advancedxy
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141696219
##########
client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java:
##########
@@ -122,4 +126,11 @@ public static void validateTestModeConf(boolean testMode, String storageType) {
+ "because of the poor performance of these two types.");
}
}
+
+ public static void validateClientType(String clientType) {
Review Comment:
Could we convert this method to `CheckValue#checkValueFunc` like `ConfigUtil#POSITIVE_LONG_VALIDATOR`?
##########
common/src/main/java/org/apache/uniffle/common/config/RssConf.java:
##########
@@ -645,4 +646,8 @@ public String getEnv(String key) {
return System.getenv(key);
}
+ @VisibleForTesting
+ public Map<String, Object> getSettings() {
Review Comment:
Could we remove this method? It seems unused.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141731590
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +124,30 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ RssConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ Map<String, Object> originConf = shuffleServerConf.getSettings();
+ Class<RssConf> clazz = RssConf.class;
+ Field field = clazz.getDeclaredField("settings");
+ field.setAccessible(true);
+ originConf.remove(ShuffleServerConf.NETTY_SERVER_PORT.key());
+ field.set(shuffleServerConf, originConf);
Review Comment:
We don't need set the value again.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141033563
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +122,29 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ ShuffleServerConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ shuffleServerConf.setInteger("rss.rpc.server.port", SHUFFLE_SERVER_PORT + 3);
+ shuffleServerConf.setInteger("rss.jetty.http.port", 18089);
+ shuffleServerConf.setInteger(ShuffleServerConf.NETTY_SERVER_PORT, -3);
Review Comment:
I think you can get the `RssConf#settings` by refection and remove the config options which you don't need.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141773460
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +124,30 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ RssConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ Map<String, Object> originConf = shuffleServerConf.getSettings();
+ Class<RssConf> clazz = RssConf.class;
+ Field field = clazz.getDeclaredField("settings");
+ field.setAccessible(true);
+ originConf.remove(ShuffleServerConf.NETTY_SERVER_PORT.key());
+ field.set(shuffleServerConf, originConf);
Review Comment:
I got it.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141731590
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +124,30 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ RssConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ Map<String, Object> originConf = shuffleServerConf.getSettings();
+ Class<RssConf> clazz = RssConf.class;
+ Field field = clazz.getDeclaredField("settings");
+ field.setAccessible(true);
+ originConf.remove(ShuffleServerConf.NETTY_SERVER_PORT.key());
+ field.set(shuffleServerConf, originConf);
Review Comment:
We don't need set the value again. They are the same object.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141031976
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +122,29 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
+ // the server's tags will be [ss_v4, GRPC_NETTY] and [ss_v4, GRPC], respectively.
+ // We need to remove the first machine's tag from GRPC_NETTY to GRPC
+ shuffleServers.get(0).stopServer();
+ ShuffleServerConf shuffleServerConf = shuffleServers.get(0).getShuffleServerConf();
+ shuffleServerConf.setInteger("rss.rpc.server.port", SHUFFLE_SERVER_PORT + 3);
+ shuffleServerConf.setInteger("rss.jetty.http.port", 18089);
+ shuffleServerConf.setInteger(ShuffleServerConf.NETTY_SERVER_PORT, -3);
Review Comment:
I need to reset netty's port number to a negative number.
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java:
##########
@@ -121,11 +122,29 @@ public void getShuffleRegisterInfoTest() {
@Test
public void getShuffleAssignmentsTest() throws Exception {
- String appId = "getShuffleAssignmentsTest";
+ final String appId = "getShuffleAssignmentsTest";
CoordinatorTestUtils.waitForRegister(coordinatorClient,2);
+ // When the shuffleServerHeartbeat Test is completed before the current test,
Review Comment:
https://github.com/apache/incubator-uniffle/blob/1021031b5eb1f4b97448f6431ea23e13a691b874/integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java#L249
Because this `shuffleServerConf` modifies the netty port number of a server, there will be one less machine allocated. Therefore, in order to maintain the original test, we need to set the port number back to a negative number, which is equivalent to setting the label to `GRPC`.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141008316
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -248,9 +249,18 @@ private void initServerTags() {
if (CollectionUtils.isNotEmpty(configuredTags)) {
tags.addAll(configuredTags);
}
+ isNettyServerEnabled();
LOG.info("Server tags: {}", tags);
}
+ private void isNettyServerEnabled() {
Review Comment:
What about `taggedServer`?
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141032063
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -400,7 +400,6 @@ public class ShuffleServerConf extends RssBaseConf {
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")
Review Comment:
You can see https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141031976 and https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141031669
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141315283
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -130,6 +131,11 @@ public static void main(String[] args) {
assignmentTags.addAll(Arrays.asList(rawTags.split(",")));
}
assignmentTags.add(Constants.SHUFFLE_SERVER_VERSION);
+ if (ClientType.GRPC_NETTY.name().equals(conf.get(RssMRConfig.RSS_CLIENT_TYPE))) {
Review Comment:
Could we also add a conf validator for RSS_CLIENT_TYPE here?
Then I think we can simply call `assignmentTags.add(conf.get(RssMRConfig.RSS_CLIENT_TYPE))` 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] jerqi commented on a diff in pull request #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1140977166
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -248,9 +249,18 @@ private void initServerTags() {
if (CollectionUtils.isNotEmpty(configuredTags)) {
tags.addAll(configuredTags);
}
+ isNettyServerEnabled();
LOG.info("Server tags: {}", tags);
}
+ private void isNettyServerEnabled() {
Review Comment:
Could we have a better name?
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141023512
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -400,7 +400,6 @@ public class ShuffleServerConf extends RssBaseConf {
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")
Review Comment:
Why do we remove this 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 a diff in pull request #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141730395
##########
common/src/main/java/org/apache/uniffle/common/config/RssConf.java:
##########
@@ -645,4 +646,8 @@ public String getEnv(String key) {
return System.getenv(key);
}
+ @VisibleForTesting
+ public Map<String, Object> getSettings() {
Review Comment:
We have already used reflection there, we don't need add this method, we can get this value by reflection.
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141012889
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -346,6 +347,11 @@ public <K, V, C> ShuffleHandle registerShuffle(int shuffleId, ShuffleDependency<
long retryInterval = sparkConf.get(RssSparkConfig.RSS_CLIENT_ASSIGNMENT_RETRY_INTERVAL);
int retryTimes = sparkConf.get(RssSparkConfig.RSS_CLIENT_ASSIGNMENT_RETRY_TIMES);
int estimateTaskConcurrency = RssSparkShuffleUtils.estimateTaskConcurrency(sparkConf);
+ if (sparkConf.get(RssSparkConfig.RSS_CLIENT_TYPE).equals(ClientType.GRPC_NETTY.name())) {
Review Comment:
More safe
```
ClientType.GRPC_NETTY.name().equals(sparkConf.get(RssSparkConfig.RSS_CLIENT_TYPE))
```
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739
--
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 #739: [#719] feat(netty): Optimize allocation strategy
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #739:
URL: https://github.com/apache/incubator-uniffle/pull/739#discussion_r1141747214
##########
client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java:
##########
@@ -122,4 +126,11 @@ public static void validateTestModeConf(boolean testMode, String storageType) {
+ "because of the poor performance of these two types.");
}
}
+
+ public static void validateClientType(String clientType) {
Review Comment:
It doesn't seem to work because there is no `checkValue` on mr's client side.
--
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