util/log: refactor and bug fixes #40993
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
20190323-util-log-refactor
branch
from
c319c44
to
be64598
Compare
craig bot
pushed a commit
that referenced
this pull request
Sep 24, 2019
41018: c-deps: bump rocksdb for unique cache IDs on Windows r=ajkr a=ajkr Picks up cockroachdb/rocksdb#58. We found a corruption caused by multiple FAT32 files assigned the same block cache key prefix. We don't know the extent to which this problem affects other filesystems or other Windows file ID generation mechanisms. We decided to turn off the reliance on filesystem for generating cache keys on Windows. Instead we use randomization per table reader. This would cause a performance penalty for use cases that open multiple table readers per file, but I believe cockroach is not such a use case. Fixes #40918, fixes #40950. Release justification: Prevents corruption on some Windows filesystems Release note: None 41020: util/log: fix GC of secondary loggers r=petermattis a=knz Fixes #40974. This is a subset of #40993 suitable for 19.2 and backport to 19.1. Release justification: bug fix Release note (bug fix): CockroachDB will now properly remove excess secondary log files (SQL audit logging, statement execution logging, and RocksDB events). Co-authored-by: Andrew Kryczka <andrew.kryczka2@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
This was referenced Oct 1, 2019
knz
force-pushed
the
20190323-util-log-refactor
branch
from
be64598
to
340846a
Compare
|
Added a commit to use |
craig bot
pushed a commit
that referenced
this pull request
Oct 1, 2019
41220: sql: move histogram decoding into the stats cache r=RaduBerinde a=RaduBerinde Currently the stats cache exposes the encoded histogram data, and the opt catalog decodes it. The problem is that opt catalog objects are not shared across connections so this is inefficient (especially in memory usage). This change moves the decoding into the stats cache. The opt catalog objects now simply point to the data computed by the cache. There are still inefficiencies to improve on in the future: the opt catalog can hold on to multiple versions of tables, so we will keep many versions of histograms "alive". Informs #41206. Informs #40922. Release justification: fix for new functionality. Release note: None 41231: util/log: ensure that secondary loggers do not leak memory r=petermattis,nvanbenschoten a=knz Fixes #41230. Note: the primary cause of this issue is removed by #40993 but that PR is blocked until 19.2 is out. I'm cherry-picking the subset of those changes sufficient to solve issue #41230, here. Prior to this patch, logging via a secondary logger would allocate a buffer, then add it to the buffer free list of the secondary logger. This was causing a memory leak because only the free list from the main logger is used to allocate buffers (even in secondary loggers), so all the now-unused buffers from secondary logs would remain unused and accumulate, locked from Go's GC attention because they are referenced somewhere. Release justification: bug fix Release note (bug fix): A memory leak was fixed that affected secondary logging (SQL audit logs, statement execution, and RocksDB logging). Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
knz
force-pushed
the
20190323-util-log-refactor
branch
from
340846a
to
74eb436
Compare
knz
force-pushed
the
20190323-util-log-refactor
branch
from
74eb436
to
56e5ad2
Compare
petermattis
reviewed
Oct 14, 2019
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz, @petermattis, and @tbg)
pkg/util/log/clog.go, line 43 at r3 (raw file):
// pool for entry formatting buffers. bufPool sync.Pool
I would make this a global. All of the loggers should be sharing a single sync.Pool.
This change is functionality-neutral and performs the following code
structure adjustments:
- renames `logging` to `mainLog` to reflect its true nature.
- moves code away from `clog.go` to separate files:
- `severity.go`: the `Severity` printing and conversion routines.
- `log_entry.go`: code specific to building and parsing log entries
(i.e. conversion between text and `log.Entry`).
- `log_gc.go`: removing old log files (files beyond the configured
size and number thresholds).
- `log_flush.go`: the code that handles flushing the buffered log data
to files and output streams.
- `log_bridge.go`: the code that manages the coupling of the
cockroachdb `log` package with the default Go `log` package.
- `sync_buffer.go`: the code that manages the coupling between
a logger and an `*os.File`.
- `log_buffer.go`: the code that manages buffering of a single log
entry, and optimizes the reuse of entry buffers.
- `vmodule.go`: the code that filters the production of log entries
depending on the source code point where the entry is produced,
depending on `--vmodule` and `crdb_internal.set_vmodule()`
configurations.
- `exit_override.go`: the code that manages the callback when the
logging system is shutting down.
- `get_stacks.go`: the code that assembles the goroutine stack dumps
to produce upon a log.Fatal entry.
- `intercept.go`: the code that injects a callback for log events.
(Used by the `logspy` debug endpoint)
- `backtrace_trigger.go`: the code that manages
the special purpose tracepoints.
- `tracebacks.go`: the code that manages the the coupling with
the standard Go environment variable `GOTRACEBACK`.
Release note: None
This commit introduces the distinction between `loggingT`, the global logging state, and `loggerT`, the per-logger state. The main logger also becomes a global variable `mainLog` in this package. With this commit, the mutex on the global state is not held any more on the common path, which means that there will be less contention between primary and secondary loggers, and between secondary loggers, if at all. Release note (bug fix): CockroachDB will now properly remove excess secondary log files (SQL audit logging, statement execution logging, and RocksDB events). Release note (bug fix): CockroachDB will now properly emit the cluster ID, once known, to all secondary log files (SQL audit logging, statement execution logging, and RocksDB events).
This was suggested by @petermattis: the free list code predates the introduction of `sync.Pool` in the Go standard library. Using `sync.Pool` ensures that the code becomes less sensitive to a missing call to `putBuffer()`. Release note: None
knz
force-pushed
the
20190323-util-log-refactor
branch
from
56e5ad2
to
f2bae17
Compare
knz
commented
Oct 16, 2019
There was a problem hiding this comment.
Reviewable status:
pkg/util/log/clog.go, line 43 at r3 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I would make this a global. All of the loggers should be sharing a single
sync.Pool.
It is global now. The loggingT is now used for the common singleton, and there is a separate per-logger loggerT.
petermattis
reviewed
Oct 16, 2019
There was a problem hiding this comment.
Reviewable status:
pkg/util/log/clog.go, line 43 at r3 (raw file):
Previously, knz (kena) wrote…
It is global now. The
loggingTis now used for the common singleton, and there is a separate per-loggerloggerT.
Ah, I missed that. Very good. Carry on.
tbg
approved these changes
Oct 17, 2019
There was a problem hiding this comment.
Reviewed 7 of 26 files at r1, 20 of 20 files at r4, 18 of 18 files at r5, 2 of 2 files at r6.
Reviewable status:
|
thank you! bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Oct 17, 2019
40993: util/log: refactor and bug fixes r=knz a=knz Fixes #40983. Fixes #40973. Fixes #40974. Fixes #41231. I might also want to add code to clean up #40972, #40982 and #40990 provided I receive some guidance from @tbg or someone familiar with the log package. 41654: parser: fix telemetry link for materialized views r=jordanlewis a=jordanlewis This commit fixes the error message that you get if you try to create a materialized view to point at the right GitHub issue. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Build succeeded |
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.
Fixes #40983.
Fixes #40973.
Fixes #40974.
Fixes #41231.
I might also want to add code to clean up #40972, #40982 and #40990 provided I receive some guidance from @tbg or someone familiar with the log package.