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 2023/08/11 13:27:54 UTC
[GitHub] [incubator-celeborn] waitinfuture opened a new pull request, #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has c…
waitinfuture opened a new pull request, #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808
…losed to avoid data lost
<!--
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?
This PR fixes a bug that in rare cases it may cause data lost.
### Why are the changes needed?
I received a bug report from one of the users that in an extreme case small data lost happens. I
reproduced the bug under the following conditions:
1. Shuffle data size for one partition id is relatively large, for example 400GB
2. `celeborn.client.shuffle.partitionSplit.mode` is set to HARD
3. `celeborn.client.shuffle.batchHandleCommitPartition.enabled` is enabled
At the mean time, there are warning messages in worker's log
```
23/08/11 17:10:04,501 WARN [push-server-6-44] PushDataHandler: Append data failed for task(shuffle application_1691635581416_0021-0, map 746, attempt 0), caused by AlreadyClosedException, endedAttempt -1, error message: FileWriter has already closed!, fileName /mnt/disk1/celeborn-worker/shuffle_data/application_1691635581416_0021/0/0-107-0
23/08/11 17:12:04,445 WARN [push-server-6-35] PushDataHandler: Append data failed for task(shuffle application_1691635581416_0021-0, map 3016, attempt 0), caused by AlreadyClosedException, endedAttempt -1, error message: FileWriter has already closed!, fileName /mnt/disk3/celeborn-worker/shuffle_data/application_1691635581416_0021/0/0-356-0
```
After digging into it, I found the reason for the data lost is as follows:
1. For some partition id in some worker, the file size exceeds `celeborn.client.shuffle.partitionSplit.threshold`, then
`CommitManager` in `LifecycleManager` will trigger `CommitFiles` because `batchHandleCommitPartition` is enabled
2. Before `CommitFile` finishes, `PushDataHandler` receives `PushData` or `PushMergedData`, it finds that the partition has not committed yet, and is preparing to call `fileWriter.incrementPendingWrites()` and `callback.onSuccess`
3. Before `PushDataHandler` calls `fileWriter.incrementPendingWrites()`, the `CommitFiles` finishes and the FileWriter
successfully closes.
4. Then `PushDataHandler` calls `fileWriter.incrementPendingWrites()` and `callback.onSuccess`. After this time,
`ShuffleClient` thinks the `PushData` succeeds. However, when `PushDataHandler` calls `fileWriter.write()`, it
finds it already closed and throws the above exception. However, the exception is ignored, so the data lost happens.
This PR fixes this by checking whether FileWriter has closed after calling `incrementPendingWrites`. If true,
`PushDataHandler` calls `onFailure`.
### 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
[GitHub] [incubator-celeborn] waitinfuture closed pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has closed to avoid data lost
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has closed to avoid data lost
URL: https://github.com/apache/incubator-celeborn/pull/1808
--
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
[GitHub] [incubator-celeborn] cfmcgrady commented on a diff in pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has closed to avoid data lost
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808#discussion_r1291595894
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -499,6 +508,15 @@ class PushDataHandler extends BaseMessageHandler with Logging {
}
fileWriters.foreach(_.incrementPendingWrites())
+ val closedFileWriter = fileWriters.find(_.isClosed)
Review Comment:
sorry for the later review, I think we should use the `while` loop, like what we did in https://github.com/apache/incubator-celeborn/pull/1668
--
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
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has closed to avoid data lost
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808#issuecomment-1675034969
Thanks, merging to main/0.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
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has c…
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808#issuecomment-1674810328
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1808?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1808](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1808?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a14389b) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/54622814cc750854500075d9d29aaf2ff7a27d9c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5462281) will **increase** coverage by `0.04%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1808 +/- ##
==========================================
+ Coverage 46.73% 46.77% +0.04%
==========================================
Files 162 162
Lines 10078 10078
Branches 927 927
==========================================
+ Hits 4709 4713 +4
+ Misses 5060 5058 -2
+ Partials 309 307 -2
```
[see 2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1808/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=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
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has c…
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808#issuecomment-1674829716
In my testing of this PR, the added log appears in Worker's logs:
```
[7] 21:50:50 [SUCCESS] core-1-5
23/08/11 21:49:42,919 WARN [push-server-6-63] PushDataHandler: [handlePushMergedData] FileWriter is already closed! File path /mnt/disk4/celeborn-worker/shuffle_data/application_1691756421542_0004/0/0-763-0
[8] 21:50:50 [SUCCESS] core-1-4
23/08/11 21:50:02,920 WARN [push-server-6-9] PushDataHandler: [handlePushMergedData] FileWriter is already closed! File path /mnt/disk1/celeborn-worker/shuffle_data/application_1691756421542_0004/0/0-958-0
[emr-user@master-1-1(192.168.80.193) ~]$ pssh -h hosts -i 'cat /home/emr-user/apache-celeborn-0.4.0-SNAPSHOT-bin/logs/celeborn.log |grep "FileWriter is already closed"'
```
--
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
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has c…
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808#issuecomment-1674796394
cc @JQ-Cao @zy-jordan @pan3793 @AngersZhuuuu @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
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1808: [CELEBORN-890][BUG] PushHandler should check whether FileWriter has closed to avoid data lost
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1808:
URL: https://github.com/apache/incubator-celeborn/pull/1808#issuecomment-1674848400
Thanks, merging to main/0.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