You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "waitinfuture (via GitHub)" <gi...@apache.org> on 2024/01/17 10:49:26 UTC
[PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
waitinfuture opened a new pull request, #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
I tested 1T TPCDS with the following Celeborn 8-worker cluster setup:
1. Workers have fixed ports for rpc/push/replicate
2. `spark.celeborn.client.spark.fetch.throwsFetchFailure` is enabled
3. graceful shutdown is enabled
I randomly kill -9 some workers (so that graceful shutdown will not work) and start it immediately.
And I encountered result incorrectness with low probability (1 out of 99 queries).
After digging into it, I found the reason is as follows:
1. At time T1, all workers are serving shuffle 602
2. At time T2, I killed -9 and start worker1 and worker2. Since the workers are configured with fixed ports,
clients think they are OK and Master let them re-register, which will also success. And the worker is clean in memory.
4. At time T3, push requests to worker2 fails and revives on worker1, so worker1 has reservation for shuffle 602
5. At time T4, LifecycleManager sends CommitFiles to all workers, on worker1, it just logs that some PartitionLocations
don't exist and ignores them.
6. CommitFiles success, but worker1 loses some data before restarting.
The following snapshot shows the process.
This PR fixes this by treating unfound PartitionLocations as failed when handling CommitFiles.
### Why are the changes needed?
ditto
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Manual test
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#issuecomment-1895581333
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2235?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`30608ea`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/30608ea698de7c068a50df5f22dd027764ede810?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.61% compared to head [(`5341918`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2235?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.59%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2235 +/- ##
==========================================
- Coverage 47.61% 47.59% -0.01%
==========================================
Files 192 192
Lines 11865 11865
Branches 1050 1050
==========================================
- Hits 5648 5646 -2
- Misses 5843 5844 +1
- Partials 374 375 +1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2235?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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#issuecomment-1895558442
cc @RexXiong @FMX
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2235: [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles
URL: https://github.com/apache/incubator-celeborn/pull/2235
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#issuecomment-1897724552
Thanks, merging to main(v0.5.0)/branch-0.4(v0.4.0)/branch-0.3(v0.3.3)
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#discussion_r1455783901
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -3325,6 +3326,15 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
+ val TEST_WORKER_UNDER_TEST: ConfigEntry[Boolean] =
Review Comment:
why this configuration needed?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#discussion_r1456715217
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -3325,6 +3326,15 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
+ val TEST_WORKER_UNDER_TEST: ConfigEntry[Boolean] =
Review Comment:
The reason is that WithShuffleClientSuite#registerAndFinishPartition repeatedly called mapPartitionMapperEnd twice. I reverted the config and removed the duplicate invocation. PTAL @RexXiong
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#discussion_r1456715217
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -3325,6 +3326,15 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
+ val TEST_WORKER_UNDER_TEST: ConfigEntry[Boolean] =
Review Comment:
The reason is that WithShuffleClientSuite#registerAndFinishPartition repeatedly called mapPartitionMapperEnd twice. I reverted the config and removed the duplicate invocation.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1233] Treat unfound PartitionLocation as failed in Controller#commitFiles [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2235:
URL: https://github.com/apache/incubator-celeborn/pull/2235#discussion_r1456715217
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -3325,6 +3326,15 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
+ val TEST_WORKER_UNDER_TEST: ConfigEntry[Boolean] =
Review Comment:
Some UTs will fail without this config, they don't really call commitFiles.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org