cli: avoid using background ctx in server start #73678
Merged
Conversation
This file contains 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
|
This change is |
knz
force-pushed
the
20211110-start-ctx
branch
2 times, most recently
from
2ba1f47 to
17ef92b
Compare
RaduBerinde
approved these changes
Dec 10, 2021
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)
pkg/cli/start.go, line 708 at r5 (raw file):
// We'll want to log any shutdown activity against a separate span. var shutdownSpan *tracing.Span
[nit] Is this line needed?
knz
force-pushed
the
20211110-start-ctx
branch
2 times, most recently
from
75941fe to
9f86eb8
Compare
knz
commented
Dec 13, 2021
There was a problem hiding this comment.
Reviewable status:
pkg/cli/start.go, line 708 at r5 (raw file):
Previously, RaduBerinde wrote…
[nit] Is this line needed?
Nope, removed.
|
TFYRs! bors r=RaduBerinde,cameronnunez |
|
Build failed: |
This change ensures that the entire server startup and shutdown logic operates under a go context that is connected to the main config's tracer and AmbientCtx (and its log tags, and server identifiers). Release note: None
knz
force-pushed
the
20211110-start-ctx
branch
from
9f86eb8 to
359dedb
Compare
|
lint fail |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Dec 14, 2021
72798: util: support 128 FastIntSet elements without allocation r=RaduBerinde a=RaduBerinde #### util: support 128 FastIntSet elements without allocation This commit increases the size of the "small set" bitmap from 64 bits to 128 bits. The main goal is to avoid the slow path in optimizer ColSets for more queries. Fixes #72733. Release note: None 73306: log,server: avoid global variables to log/trace server IDs r=RaduBerinde,cameronnunez a=knz First commit from #73678. Fixes #58938. (still incomplete - needs all the other PRs connected to that issue) Prior to this change, the server identifiers (cluster ID, node ID etc) were stored in global variables in the `log` package. This was problematic when a single process contains multiple servers, e.g. in tests, `demo` and multi-tenant CockroachDB. This change switches the mechanism to use identifiers stored in the go context. The disadvantage is that the server IDs are not any more logged at the beginning of each log file (since a given log file could report data from multiple servers). Release note (cli change): The server identifiers (cluster ID, node ID, tenant ID, instance ID) are not any more duplicated at the start of every new log file (during log file rotations). They are now only logged when known during server start-up. (The copy of the identifiers is still included in per-event envelopes for the various `json` output logging formats.) Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
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.
Needed for #73306.
Informs #58938.
This change ensures that the entire server startup and shutdown logic
operates under a go context that is connected to the main config's
tracer and AmbientCtx (and its log tags, and server identifiers).