-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
stmtdiagnostics: use background context when building the bundle #108071
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the build issues are fixed. Thanks for the fix!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
pkg/sql/instrumentation.go
line 469 at r1 (raw file):
switch ih.outputMode { case explainAnalyzeDebugOutput: return setExplainBundleResult(ctx, res, bundle, cfg, warnings)
Maybe we should use the original context here and below?
When the context is canceled, we still want to build the bundle as best as possible. Over in 532274b we introduced the usage of the background context in order to insert the bundle into the system tables, but we still built the bundle with the canceled context. This commit fixes that oversight - in particular, we should now get `env.sql` correctly. Epic: None Release note: None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
bors r+
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @michae2 and @rharding6373)
pkg/sql/instrumentation.go
line 469 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Maybe we should use the original context here and below?
I missed these usages, yeah, the original context seems right here. Done.
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 7d9a30f to blathers/backport-release-22.2-108071: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.2.x failed. See errors above. error creating merge commit from 7d9a30f to blathers/backport-release-23.1-108071: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
When the context is canceled, we still want to build the bundle as best as possible. Over in 532274b we introduced the usage of the background context in order to insert the bundle into the system tables, but we still built the bundle with the canceled context. This commit fixes that oversight - in particular, we should now get
env.sql
correctly.Informs: https://github.com/cockroachlabs/support/issues/2494.
Epic: None
Release note: None