feat(bin): flush output to stdout before db conn shutdown #262
Conversation
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
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that all output is flushed to stdout before shutting down the database connection by refactoring how output is handled. Key changes include:
- Introducing a new Output trait with implementations for Stdout and Vec.
- Refactoring both serial and parallel test runners to use the new Output trait and removing buffer flushing from the caller.
- Adjusting the signature and usage of connect_and_run_test_file accordingly.
Comments suppressed due to low confidence (1)
sqllogictest-bin/src/main.rs:669
- [nitpick] Consider renaming the trait 'Output' to a more descriptive name like 'FlushableOutput' to better convey its purpose.
trait Output: Write {
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
74068a0 to
e117b19
Compare
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
skyzh
approved these changes
May 7, 2025
sqllogictest-bin/src/main.rs
Outdated
| /// to avoid interleaving output from different parallelism. | ||
| impl Output for Vec<u8> { | ||
| fn finish(&mut self) -> io::Result<()> { | ||
| let mut stdout = stdout(); |
There was a problem hiding this comment.
This does not guarantee the exclusiveness of the handler?
The std doc says,
Each handle returned is a reference to a shared global buffer whose access is synchronized via a mutex. If you need more explicit control over locking, see the Stdout::lock method.
There was a problem hiding this comment.
Good catch. However, it appears that write_all is not interruptable and we only call it once, so we still won't observe interleaving output. Nonetheless, lock seems always better.
stdrc
approved these changes
May 7, 2025
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
...so that upon cancellation, we get more prompt results from ongoing tests.