You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "yl09099 (via GitHub)" <gi...@apache.org> on 2024/12/09 07:43:46 UTC
[PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
yl09099 opened a new pull request, #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281
### What changes were proposed in this pull request?
If the current Reader fails to obtain Shuffle data, it does not trigger the upstream Stage to rewrite the data. If a Shuffle Server fails, it does not trigger Stage retry.
### Why are the changes needed?
Fix: https://github.com/apache/incubator-uniffle/issues/2279
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
UT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1886065696
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
Review Comment:
This method has a weird return value.
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1888507937
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
Review Comment:
It's not proper name if we don't change the method 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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1888641032
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
+ int stageAttempt, int partition, RssShuffleManagerInterface shuffleManager) {
return withReadLock(
() -> {
if (this.stageAttempt != stageAttempt) {
- return 0;
+ return false;
} else {
- return this.partitions[partition];
+ if (this.partitions[partition] >= shuffleManager.getMaxFetchFailures()) {
+ return true;
+ } else {
+ return false;
+ }
}
});
}
+
+ public void clearedMapTrackerBlock(boolean isCleared) {
+ withWriteLock(
+ () -> {
+ this.isClearedMapTrackerBlock = isCleared;
+ return null;
+ });
+ }
+
+ public boolean isClearedMapTrackerBlock() {
Review Comment:
The above has been changed.
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#issuecomment-2550191653
cc @maobaolong
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#issuecomment-2527222952
## Test Results
2 966 files +23 2 966 suites +23 6h 26m 34s ⏱️ + 7m 36s
1 096 tests ± 0 1 094 ✅ + 1 2 💤 ±0 0 ❌ - 1
13 735 runs +53 13 705 ✅ +54 30 💤 ±0 0 ❌ - 1
Results for commit 69f340f6. ± Comparison against base commit 0552d3b0.
[test-results]:data:application/gzip;base64,H4sIAJGmVmcC/02MNw7DMBAEvyKwdsEsnD9jMAKEJdFgqAz/3VSiVM7c7XyRD5PL6DlQkPIxoFxDubOtSZUQl9UwArypdi/rB8EgT3zlaszu+OXe4bPuuvAqTE3gLlxKMR0m1WWrspGJA3uVjfhye5Wdm3t143vUxHkOpQGS4BnHXhpHiPZGgZVYGeuBEsEFBa21s0JR9PsDJQf6dhQBAAA=
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1888489201
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
Review Comment:
> This method has a weird return value.
The number of failures is compared to the maximum number of failures in the configuration, and I moved the comparison logic to the internal implementation, so I only need to return whether fetchfailed is thrown.
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1888511422
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
+ int stageAttempt, int partition, RssShuffleManagerInterface shuffleManager) {
return withReadLock(
() -> {
if (this.stageAttempt != stageAttempt) {
- return 0;
+ return false;
} else {
- return this.partitions[partition];
+ if (this.partitions[partition] >= shuffleManager.getMaxFetchFailures()) {
+ return true;
+ } else {
+ return false;
+ }
}
});
}
+
+ public void clearedMapTrackerBlock(boolean isCleared) {
+ withWriteLock(
+ () -> {
+ this.isClearedMapTrackerBlock = isCleared;
+ return null;
+ });
+ }
+
+ public boolean isClearedMapTrackerBlock() {
Review Comment:
`isClearedMapTrackerBlock` -> `hasClearedMapTrackerBlock`
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#issuecomment-2540831695
> Wait a minute. I'm not ready for this.
If this is not ready to review, you can mark this as draft.
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#issuecomment-2527169186
Wait a minute. I'm not ready for this.
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1888509909
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -489,10 +509,13 @@ private static class RssShuffleStatus {
private final ReentrantReadWriteLock.WriteLock writeLock = lock.writeLock();
private final int[] partitions;
private int stageAttempt;
+ // Whether the Shuffle result has been cleared for the current number of attempts.
+ private boolean isClearedMapTrackerBlock;
Review Comment:
`isClearedMapTrackerBlock` -> `hasClearedMapTrackerBlock`
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1886065845
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
+ int stageAttempt, int partition, RssShuffleManagerInterface shuffleManager) {
return withReadLock(
() -> {
if (this.stageAttempt != stageAttempt) {
- return 0;
+ return false;
} else {
- return this.partitions[partition];
+ if (this.partitions[partition] >= shuffleManager.getMaxFetchFailures()) {
+ return true;
+ } else {
+ return false;
+ }
}
});
}
+
+ public void setClearedMapTrackerBlock(boolean isCleared) {
Review Comment:
Could you have a better method 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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#issuecomment-2540990754
> > Wait a minute. I'm not ready for this.
>
> If this is not ready to review, you can mark this as draft.
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#issuecomment-2548674380
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2281?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `23.52941%` with `26 lines` in your changes missing coverage. Please review.
> Project coverage is 52.30%. Comparing base [(`ac89c19`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/ac89c19b9803fc72ee45bb30e44a5e34330d3edd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`c58aa9f`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/c58aa9fb57dc050dcb53dc8917914f248b200e9a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 8 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2281?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...fle/shuffle/manager/ShuffleManagerGrpcService.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2281?src=pr&el=tree&filepath=client-spark%2Fcommon%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Funiffle%2Fshuffle%2Fmanager%2FShuffleManagerGrpcService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zaHVmZmxlL21hbmFnZXIvU2h1ZmZsZU1hbmFnZXJHcnBjU2VydmljZS5qYXZh) | 23.52% | [24 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2281?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2281 +/- ##
============================================
+ Coverage 51.78% 52.30% +0.52%
- Complexity 2966 3386 +420
============================================
Files 479 518 +39
Lines 22566 27900 +5334
Branches 2068 2628 +560
============================================
+ Hits 11686 14594 +2908
- Misses 10140 12345 +2205
- Partials 740 961 +221
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2281?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] [#2279] improvement(spark): Trigger the upstream rewrite when the read stage fails [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #2281:
URL: https://github.com/apache/incubator-uniffle/pull/2281#discussion_r1888510737
##########
client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java:
##########
@@ -553,16 +576,36 @@ public void incPartitionFetchFailure(int stageAttempt, int partition) {
});
}
- public int getPartitionFetchFailureNum(int stageAttempt, int partition) {
+ public boolean getPartitionFetchFailureNum(
+ int stageAttempt, int partition, RssShuffleManagerInterface shuffleManager) {
return withReadLock(
() -> {
if (this.stageAttempt != stageAttempt) {
- return 0;
+ return false;
} else {
- return this.partitions[partition];
+ if (this.partitions[partition] >= shuffleManager.getMaxFetchFailures()) {
+ return true;
+ } else {
+ return false;
+ }
}
});
}
+
+ public void clearedMapTrackerBlock(boolean isCleared) {
Review Comment:
Maybe we don't the parameter `isCleared`. It's rebundant.
--
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