You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2022/08/04 07:44:24 UTC
[GitHub] [incubator-uniffle] smallzhongfeng opened a new pull request, #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
smallzhongfeng opened a new pull request, #125:
URL: https://github.com/apache/incubator-uniffle/pull/125
…ffleManager
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://github.com/Tencent/Firestorm/blob/master/CONTRIBUTING.md
2. Ensure you have added or run the appropriate tests for your PR
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]XXXX Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
If `spark.sql.adaptive.enabled` is on, throw an exception instead of pushing it down.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
When AE is enabled in spark app, `DelegationRssShuffleManager` should not be directly pushed down to `SortShuffleManager`, but exceptions should be thrown out, because the main purpose of our push down is to prevent RSS resources from being busy.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released versions or within the unreleased branches such as master.
If no, write 'No'.
-->
The app will fail, informing the user that AE needs to be turned off.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
No 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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937523206
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
```
public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
throw new IllegalArgumentException("Spark2 doesn't support AQE, spark.sql.adaptive.enabled should be false.");
}
this.sparkConf = sparkConf;
```
When initializing `RssShuffleManager`, if AE is opened, an exception will be thrown here. `DelegationRssShuffleManager.createShuffleManagerInDriver()` catches the exception, prints the relevant log, and then pushes it down to`SortShuffleManager`. I think this is unreasonable.@jerqi
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937540935
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> ```
> public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
> if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
> throw new IllegalArgumentException("Spark2 doesn't support AQE, spark.sql.adaptive.enabled should be false.");
> }
> this.sparkConf = sparkConf;
> ```
>
> When initializing `RssShuffleManager`, if AE is opened, an exception will be thrown here. `DelegationRssShuffleManager.createShuffleManagerInDriver()` catches the exception, prints the relevant log, and then pushes it down to`SortShuffleManager `. I think this is unreasonable.@jerqi
RssShuffleManager don't support some parameters, we choose to use SortShuffleManager. If some Spark 2.4 client which back port AQE feature, SortShuffleManager will work well. Checking the parameters of `SortShuffleManager` isn't the responsibility of DelegationRssShuffleManager. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937833331
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
Well, I agree with you in the end. Thanks @jerqi
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937679362
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> 1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from insufficient resources, and users will gladly accept it; 2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see; 3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
We hope the class `DelegationRssShuffleManager` make people use RSS seamlessly, if the application don't satisfy the condition, it will use SortShuffleManager, not throw an exception.
You can see Alibaba EMR RSS, they have the same mind
https://github.com/alibaba/RemoteShuffleService/blob/cf2b895afbd4cee6b9a3679b811307f4331411a3/client-spark/shuffle-manager-2/src/main/scala/org/apache/spark/shuffle/rss/RssShuffleFallbackPolicyRunner.scala#L26
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937522454
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
```
public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
throw new IllegalArgumentException("Spark2 doesn't support AQE, spark.sql.adaptive.enabled should be false.");
}
this.sparkConf = sparkConf;
```
When initializing `RssShuffleManager`, if AE is opened, an exception will be thrown here. `DelegationRssShuffleManager.createShuffleManagerInDriver()` catches the exception, prints the relevant log, and then pushes it down to`SortShuffleManager `. I think this is unreasonable.@jerqi
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937607008
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> I don't think this is very reasonable
>
> 1. Since users choose to use RSS services, it is on the premise of providing services. When some parameters are not supported, let the user choose by themselves.
> 2. The reason for using RSS service is the performance bottleneck of `SortShuffleManager`. More than AE, we need the performance benefits that RSS provides.
DelegationRssShuffleManager means that if user can't use RSS because of some reasons, user accepts to use SortShuffleManager.
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937607008
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> I don't think this is very reasonable
>
> 1. Since users choose to use RSS services, it is on the premise of providing services. When some parameters are not supported, let the user choose by themselves.
> 2. The reason for using RSS service is the performance bottleneck of `SortShuffleManager`. More than AE, we need the performance benefits that RSS provides.
DelegationRssShuffleManager means that if user can't use RSS because of some reasons, user accepts to use SortShuffleManager. When user use the DelegationRssShuffleManager, the user should have made such decision. If users don't accept that, he should use RssShuffleManager directly.
--
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 closed pull request #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng closed pull request #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
URL: https://github.com/apache/incubator-uniffle/pull/125
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937598566
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
I don't think this is very reasonable
1. Since users choose to use RSS services, it is on the premise of providing services. When some parameters are not supported, let the user choose by themselves.
2. The reason for using RSS service is the performance bottleneck of `SortShuffleManager`. More than AE, we need the performance benefits that RSS provides.
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937471091
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
Why do we check the parameter for SortShuffleManager? We should have the same behavior as the origin Spark, so we shouldn't add extra check for SortShuffleManager.
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937679362
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> 1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from insufficient resources, and users will gladly accept it; 2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see; 3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
We hope the class `DelegationRssShuffleManager` make people use RSS seamlessly, if the application don't satisfy the condition, it will use SortShuffleManager, not throw an exception.
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#issuecomment-1204907998
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/125?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 [#125](https://codecov.io/gh/apache/incubator-uniffle/pull/125?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d1a5c4d) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/fd8ccdd920296745b7f27e3f36ed06238b0f274f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fd8ccdd) will **decrease** coverage by `1.49%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #125 +/- ##
============================================
- Coverage 57.17% 55.67% -1.50%
+ Complexity 1201 1122 -79
============================================
Files 150 141 -9
Lines 8178 7687 -491
Branches 773 742 -31
============================================
- Hits 4676 4280 -396
+ Misses 3256 3171 -85
+ Partials 246 236 -10
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/125?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/uniffle/coordinator/CoordinatorServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JTZXJ2ZXIuamF2YQ==) | `63.09% <0.00%> (-5.96%)` | :arrow_down: |
| [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9EYXRhU2tpcHBhYmxlUmVhZEhhbmRsZXIuamF2YQ==) | `81.25% <0.00%> (-3.13%)` | :arrow_down: |
| [...org/apache/uniffle/server/ShuffleFlushManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlRmx1c2hNYW5hZ2VyLmphdmE=) | `76.70% <0.00%> (-1.71%)` | :arrow_down: |
| [.../org/apache/spark/shuffle/writer/WriterBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JpdGVyQnVmZmVyLmphdmE=) | | |
| [...e/spark/shuffle/reader/RssShuffleDataIterator.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9yZWFkZXIvUnNzU2h1ZmZsZURhdGFJdGVyYXRvci5qYXZh) | | |
| [...k/shuffle/writer/WrappedByteArrayOutputStream.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JhcHBlZEJ5dGVBcnJheU91dHB1dFN0cmVhbS5qYXZh) | | |
| [...org/apache/spark/shuffle/writer/AddBlockEvent.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQWRkQmxvY2tFdmVudC5qYXZh) | | |
| [...che/spark/shuffle/writer/BufferManagerOptions.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQnVmZmVyTWFuYWdlck9wdGlvbnMuamF2YQ==) | | |
| [.../java/org/apache/spark/shuffle/RssSparkConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya0NvbmZpZy5qYXZh) | | |
| [...ava/org/apache/spark/shuffle/RssShuffleHandle.java](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTaHVmZmxlSGFuZGxlLmphdmE=) | | |
| ... and [2 more](https://codecov.io/gh/apache/incubator-uniffle/pull/125/diff?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: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937761769
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
You can see https://github.com/alibaba/RemoteShuffleService/blob/ba5920acdeb700ffe171dc8381794c6321ed80b8/client-spark/shuffle-manager-2/src/main/scala/org/apache/spark/shuffle/rss/RssShuffleFallbackPolicyRunner.scala#L63
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937675054
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from lacking of resources resources, and users will gladly accept it;
2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see;
3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937540935
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> ```
> public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
> if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
> throw new IllegalArgumentException("Spark2 doesn't support AQE, spark.sql.adaptive.enabled should be false.");
> }
> this.sparkConf = sparkConf;
> ```
>
> When initializing `RssShuffleManager`, if AE is opened, an exception will be thrown here. `DelegationRssShuffleManager.createShuffleManagerInDriver()` catches the exception, prints the relevant log, and then pushes it down to`SortShuffleManager `. I think this is unreasonable.@jerqi
When RssShuffleManager don't support some parameters, we choose to use SortShuffleManager. It's a reasonable logic. If some Spark 2.4 client which back port AQE feature, SortShuffleManager will work well. Checking the parameters of `SortShuffleManager` isn't the responsibility of DelegationRssShuffleManager. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937675054
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from insufficient resources, and users will gladly accept it;
2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see;
3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS.
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937754244
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
Of course, we have also used EMR RSS, because they support AE in spark2.4. We have also discussed with them about rolling back to `SortShuffleManager`. They implement this function more in consideration of RSS cluster load pressure including limiting the number of partitions, but will not roll back to `SortShuffleManager` due to unsupported parameters. If we want people use RSS seamlessly, would it be better to turn off the AE by default and use the RSS service instead in `DelegationRssShuffleManager`.
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937523206
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
```
public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
throw new IllegalArgumentException("Spark2 doesn't support AQE, spark.sql.adaptive.enabled should be false.");
}
this.sparkConf = sparkConf;
```
When initializing `RssShuffleManager`, if AE is opened, an exception will be thrown here. `DelegationRssShuffleManager.createShuffleManagerInDriver()` catches the exception, prints the relevant log, and then pushes it down to`SortShuffleManager`. I think this is unreasonable.@jerqi
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937540935
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> ```
> public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
> if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
> throw new IllegalArgumentException("Spark2 doesn't support AQE, spark.sql.adaptive.enabled should be false.");
> }
> this.sparkConf = sparkConf;
> ```
>
> When initializing `RssShuffleManager`, if AE is opened, an exception will be thrown here. `DelegationRssShuffleManager.createShuffleManagerInDriver()` catches the exception, prints the relevant log, and then pushes it down to`SortShuffleManager `. I think this is unreasonable.@jerqi
When RssShuffleManager don't support some parameters, we choose to use SortShuffleManager. It's a reasonable logic. If some Spark 2.4 client which back port AQE feature, SortShuffleManager will work well. We can give some freedom to user's Spark here and don't check the parameter strictly. Finally checking the parameters of `SortShuffleManager` isn't the responsibility of DelegationRssShuffleManager. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#issuecomment-1204892747
cc @jerqi
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937675241
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from insufficient resources, and users will gladly accept it;
2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see;
3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937675054
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from insufficient resources, and users will gladly accept it;
2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see;
3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
smallzhongfeng commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937675054
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from lacking of resources, and users will gladly accept it;
2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see;
3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
--
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 #125: [Bugfix] When turn on AE, should not directly push it down to SortShu…
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #125:
URL: https://github.com/apache/incubator-uniffle/pull/125#discussion_r937679362
##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -72,6 +72,9 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException {
LOG.info("Use RssShuffleManager");
return shuffleManager;
} catch (Exception exception) {
+ if (sparkConf.getBoolean("spark.sql.adaptive.enabled", false)) {
Review Comment:
> 1、I understand that `DelegationRssShuffleManager` is an object designed to prevent various problems, but more problems should come from insufficient resources, and users will gladly accept it; 2、If a parameter is not supported and the user is unaware, a large number of tasks go to `SortShuffleManager`, causing the whole batch of tasks to run completely. This is a waste of RSS service resources and is what the user does not want to see; 3、The reason we try to access this service is to solve the shuffle performance bottleneck. Since we can accept `RssShuffleManager` to turn off AE, it means that the user prefers to use RSS. If the user wants to use the function of AE very much, It's completely possible to use the origin spark ESS instead of RSS, but I think more users choose the uniffle service to support RSS. WDYT?
We hope the class `DelegationRssShuffleManager` make people use RSS seamlessly, if the application don't satisfy the condition, it will use SortShuffleManager, not throw an exception.
You can see other Alibaba EMR RSS, they have the same mind
https://github.com/alibaba/RemoteShuffleService/blob/cf2b895afbd4cee6b9a3679b811307f4331411a3/client-spark/shuffle-manager-2/src/main/scala/org/apache/spark/shuffle/rss/RssShuffleFallbackPolicyRunner.scala#L26
--
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