Uh oh!
There was an error while loading. Please reload this page.
[fix](be) Poll packed file async close without blocking - #62938
Merged
Conversation
wyxxxcat
commented
Apr 29, 2026
CollaboratorAuthor
/review |
wyxxxcat
commented
Apr 29, 2026
CollaboratorAuthor
run buildall |
wyxxxcat
commented
Apr 29, 2026
CollaboratorAuthor
run buildall |
luwei16
previously approved these changes
Apr 29, 2026
Contributor
PR approved by at least one committer and no changes requested. |
Contributor
PR approved by anyone and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
liaoxin01
commented
Apr 29, 2026
Contributor
/review |
Contributor
There was a problem hiding this comment.
Automated review completed for PR 62938 at head 7a80931.
Findings: no additional blocking issues found. I also checked the existing inline thread about S3FileWriter::try_finish_close() line 187 being potentially unreachable and did not duplicate that already-known concern.
Critical checkpoint conclusions:
- Goal/test: The change aims to let
PackedFileManagerpoll an existing S3 async close instead of repeatedly callingclose(true). The implementation follows that goal, and the addedProcessUploadingFilesPollsAsyncCloseWithoutBlockingunit test covers the polling/failure path. - Scope: The change is small and focused on
FileWriter,S3FileWriter,PackedFileManager, and the related unit test. - Concurrency: The modified lifecycle removes the async worker's direct
_state = CLOSEDtransition and lets the polling/finish path consume the future, which is consistent with avoiding state changes from the async close thread. I did not find a new deadlock or lock-order issue in the packed-file manager path. - Lifecycle:
try_finish_close()consumes the future and clears_async_close_pack; destructor behavior remains able to wait if the async result was not consumed. No new cross-TU static initialization concern found. - Config/compatibility/protocol: No new config, storage format, or FE-BE protocol compatibility changes were introduced.
- Parallel paths: Packed-file upload uses the new hook; other
close(true)users still have the existing blockingclose(false)completion path and were not found to require this polling API. - Tests: Unit coverage was added for the new polling behavior. I did not run tests in this review environment.
- Observability: Existing packed-file and S3 writer metrics/logs remain in place; no required new metric was identified for this fix.
- Transaction/persistence/data write correctness: The PR does not change metadata persistence ordering or transaction visibility semantics. Packed-file metadata update/trailer append/upload order is unchanged.
- Performance: The change avoids repeated async close submission/poll blocking in the packed-file background loop; no new obvious hot-path inefficiency found.
User focus: .opencode-review.HPzl8E/review_focus.txt contained no additional user-provided focus points.
wyxxxcat
commented
Apr 29, 2026
CollaboratorAuthor
run buildall |
liaoxin01
approved these changes
Apr 30, 2026
liaoxin01
commented
Apr 30, 2026
Contributor
run buildall |
Contributor
PR approved by at least one committer and no changes requested. |
gavinchou
approved these changes
Apr 30, 2026
Uh oh!
There was an error while loading. Please reload this page.
github-actionsBot
pushed a commit
that referenced
this pull request
Apr 30, 2026
### What problem does this PR solve? fix: #57770 This change adds a non-blocking try_finish_close() hook to FileWriter and implements it for S3FileWriter. PackedFileManager now polls the async close result instead of calling close(true) repeatedly while packed files are uploading. The change avoids re-submitting async close work and lets packed file upload state transition only after the previous async close has actually completed. It also propagates async close failures back into the packed file state so failed uploads are recorded correctly. A ut test was added to verify that PackedFileManager polls async close without blocking or issuing an extra close call, and that async close failures are handled as packed file upload failures.
yiguolei pushed a commit
that referenced
this pull request
May 1, 2026
Closed
wyxxxcat added a commit
to wyxxxcat/doris
that referenced
this pull request
May 22, 2026
fix: apache#57770 This change adds a non-blocking try_finish_close() hook to FileWriter and implements it for S3FileWriter. PackedFileManager now polls the async close result instead of calling close(true) repeatedly while packed files are uploading. The change avoids re-submitting async close work and lets packed file upload state transition only after the previous async close has actually completed. It also propagates async close failures back into the packed file state so failed uploads are recorded correctly. A ut test was added to verify that PackedFileManager polls async close without blocking or issuing an extra close call, and that async close failures are handled as packed file upload failures.
wyxxxcat added a commit
to wyxxxcat/doris
that referenced
this pull request
May 22, 2026
fix: apache#57770 This change adds a non-blocking try_finish_close() hook to FileWriter and implements it for S3FileWriter. PackedFileManager now polls the async close result instead of calling close(true) repeatedly while packed files are uploading. The change avoids re-submitting async close work and lets packed file upload state transition only after the previous async close has actually completed. It also propagates async close failures back into the packed file state so failed uploads are recorded correctly. A ut test was added to verify that PackedFileManager polls async close without blocking or issuing an extra close call, and that async close failures are handled as packed file upload failures.
morningman pushed a commit
that referenced
this pull request
May 28, 2026
Closed
zhaorongsheng pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Jun 4, 2026
### What problem does this PR solve? fix: apache#57770 This change adds a non-blocking try_finish_close() hook to FileWriter and implements it for S3FileWriter. PackedFileManager now polls the async close result instead of calling close(true) repeatedly while packed files are uploading. The change avoids re-submitting async close work and lets packed file upload state transition only after the previous async close has actually completed. It also propagates async close failures back into the packed file state so failed uploads are recorded correctly. A ut test was added to verify that PackedFileManager polls async close without blocking or issuing an extra close call, and that async close failures are handled as packed file upload failures.
sollhui added a commit
to sollhui/doris
that referenced
this pull request
Jun 24, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#62938 Problem Summary: S3FileWriter did not fully handle async task submission failures in the upload-buffer and async-close paths. Upload tasks were added to the writer countdown before being submitted to the async upload executor, so a submit failure could hide the real error and leave later close/destructor waits stuck. Async close submit failures could also leave the writer in ASYNC_CLOSING without a valid future completion path. This change centralizes S3 upload-buffer submission so submit failures go through the normal completion callback and release the countdown. It also falls back to synchronous close when async close task submission fails. ThreadPool submission is fixed for the cold-start worker creation failure path as well: when creating the first worker fails, do_submit now returns before retaining the runnable, so callers can safely treat a non-OK submit result as not queued. A unit test covers this ThreadPool contract, and a cloud regression test under cloud_p0/s3 covers both S3 writer DBUG-injected submit-failure paths. ### Release note None ### Check List (For Author) - Test: Regression test / Unit test / Manual test - git diff --check: passed - run-be-ut.sh --run --filter=ThreadPoolTest.TestSubmitFailureDoesNotRetainTaskWithoutWorker: failed locally because OpenBLAS CMake could not find OpenMP_C - run-regression-test.sh --run -d cloud_p0/s3 -s test_s3_file_writer_submit_error: failed locally because no FE/MySQL service was listening on 127.0.0.1:9030 - build.sh --be: failed locally because OpenBLAS CMake could not find OpenMP_C - build-support/clang-format.sh for changed C++ files: failed locally because llvm@16 is not installed - Behavior changed: No - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
fix: #57770
This change adds a non-blocking try_finish_close() hook to FileWriter and implements it for S3FileWriter. PackedFileManager now polls the async close result instead of calling close(true) repeatedly while packed files
are uploading.
The change avoids re-submitting async close work and lets packed file upload state transition only after the previous async close has actually completed. It also propagates async close failures back into the packed
file state so failed uploads are recorded correctly.
A ut test was added to verify that PackedFileManager polls async close without blocking or issuing an extra close call, and that async close failures are handled as packed file upload failures.
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)