Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Betters shutdown logging #1484

Merged
merged 6 commits into from
Apr 30, 2024
Merged

Betters shutdown logging #1484

merged 6 commits into from
Apr 30, 2024

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Apr 30, 2024

Betters shutdown logging

When using #[error(transparent)] to wrap ShutdownError, the error root cause is not propagated correctly all the way down to task center. The reason is that thiserror will forward source() to the type marked with transparent. In this case, it's ShutdownError it self (and it's not a root cause for itself). The solution is to make a private marker type that's guaranteed to always be the root cause of an error chain that was caused by a shutdown.


Stack created with Sapling. Best reviewed with ReviewStack.

Copy link

Test Results

 99 files  +4   99 suites  +4   8m 11s ⏱️ +24s
 84 tests +2   82 ✅ +2  2 💤 ±0  0 ❌ ±0 
216 runs  +4  210 ✅ +4  6 💤 ±0  0 ❌ ±0 

Results for commit 345a861. ± Comparison against base commit 05eb4ba.

Copy link

Test Results

 99 files  +4   99 suites  +4   8m 37s ⏱️ +50s
 84 tests +2   82 ✅ +2  2 💤 ±0  0 ❌ ±0 
216 runs  +4  210 ✅ +4  6 💤 ±0  0 ❌ ±0 

Results for commit 345a861. ± Comparison against base commit 05eb4ba.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. +1 for merging. Thanks for sharing the insights into how error(transparent) changes the source retrieval. Didn't know this.

**IMPORTANT:** This breaks queries through datafusion until we workout how data fusion will shard queries across partitions.
When using `#[error(transparent)]` to wrap `ShutdownError`, the error root cause is not propagated correctly all the way down to task center. The reason is that `thiserror` will forward `source()` to the type marked with `transparent`. In this case, it's ShutdownError it self (and it's not a root cause for itself). The solution is to make a private marker type that's guaranteed to always be the root cause of an error chain that was caused by a shutdown.
@AhmedSoliman AhmedSoliman merged commit d1cd9d6 into main Apr 30, 2024
5 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1484 branch April 30, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants