You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "rickyma (via GitHub)" <gi...@apache.org> on 2024/03/21 09:30:25 UTC
[PR] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
rickyma opened a new pull request, #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595
### What changes were proposed in this pull request?
Fail fast when the `shuffleId` is cleaned up.
### Why are the changes needed?
A follow-up PR for: https://github.com/apache/incubator-uniffle/pull/1574
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing 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
Re: [PR] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#issuecomment-2011777299
## Test Results
2 313 files - 27 2 313 suites - 27 4h 23m 17s :stopwatch: - 8m 44s
908 tests ± 0 906 :white_check_mark: - 1 1 :zzz: ±0 1 :x: +1
10 485 runs - 56 10 470 :white_check_mark: - 57 14 :zzz: ±0 1 :x: +1
For more details on these failures, see [this check](https://github.com/apache/incubator-uniffle/runs/22924842352).
Results for commit 79c51abf. ± Comparison against base commit 32d533d2.
[test-results]:data:application/gzip;base64,H4sIAM0C/GUC/1XMSw7CIBSF4a00jB0ApTzcjIFbSIh9GAoj4969NKXW4fm4/G8S4uQ3cu94z/pbR7YS83WPJdkc1wWFDcooJHzP9cJQ3dZjKwA7yR8946v+OiHYOP2BT2lNKBQllaU2GRV6OGaLoil62lEVDS7VfV+jsM5zzDiIMjAw64IJigunLAehheDSj0GPstcGmHcSJPl8Aagu+XwSAQAA
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#issuecomment-2012154324
Could you add a simple ut for this case?
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#discussion_r1533731790
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -268,6 +270,20 @@ public void sendShuffleData(
+ spd.getPartitionId()
+ "]";
try {
+ isShuffleIdDeleted =
Review Comment:
I think a new `appId` and `shuffleId` will be already registered during the `registerShuffle` phase, which means `isShuffleIdDeleted` will be false when a new application arrives.
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#issuecomment-2012182412
> Could you add a simple ut for this case?
I don't think we need extra UTs for this? This is an improvement. We simply want it to fail fast in advance. It doesn't affect the main process, but merely avoids executing unnecessary steps. What UTs would you like to be added? I have no idea.
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma closed pull request #1595: [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up
URL: https://github.com/apache/incubator-uniffle/pull/1595
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#issuecomment-2012247751
After rethinking about this, I will close this PR first.
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#issuecomment-2011757918
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?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 `0%` with `25 lines` in your changes are missing coverage. Please review.
> Project coverage is 54.91%. Comparing base [(`32d533d`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/32d533d226346600f9ed945c81bc19bf0c7c2994?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`79c51ab`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/uniffle/server/ShuffleServerGrpcService.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyR3JwY1NlcnZpY2UuamF2YQ==) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...niffle/server/netty/ShuffleServerNettyHandler.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9uZXR0eS9TaHVmZmxlU2VydmVyTmV0dHlIYW5kbGVyLmphdmE=) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?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 #1595 +/- ##
============================================
+ Coverage 54.01% 54.91% +0.90%
Complexity 2863 2863
============================================
Files 438 418 -20
Lines 24850 22508 -2342
Branches 2114 2118 +4
============================================
- Hits 13423 12361 -1062
+ Misses 10586 9377 -1209
+ Partials 841 770 -71
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1595?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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma closed pull request #1595: [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up
URL: https://github.com/apache/incubator-uniffle/pull/1595
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1595:
URL: https://github.com/apache/incubator-uniffle/pull/1595#discussion_r1533668066
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -268,6 +270,20 @@ public void sendShuffleData(
+ spd.getPartitionId()
+ "]";
try {
+ isShuffleIdDeleted =
Review Comment:
what will happen it if server processes a new application?
--
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] [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma closed pull request #1595: [#1571][FOLLOWUP] improvement(server): Fail fast when the shuffleId is cleaned up
URL: https://github.com/apache/incubator-uniffle/pull/1595
--
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