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

release-22.2: server: write pebble log messages to storage channel #90650

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Oct 25, 2022

Backport 1/1 commits from #90478 on behalf of @renatolabs.

/cc @cockroachdb/release


Previously, when setting up the server, the pebble engine would be initialized with Pebble's default logger. The reason for this is that the pebble initialization code calls EnsureDefaults on the configuration options before checking if the options.Logger is nil/unset. At that point, it will never be unset, as EnsureDefaults will set the logger to pebble.DefaultLogger if it was not previously set.

This change overwrites the pebble logger if its found to be the DefaultLogger. We never want to use pebble's DefaultLogger in CRDB as that would mean pebble would use the standard library log package, making every message emitted by Pebble to be treated as INFO level messages, regardless of severity (including log.Fatal calls).

Related to #83079.

Fixes #72683.
Fixes #90483.

Release note: None.


Release justification: fixes a bug where pebble crashes could occur without any error messages being logged.

Previously, when setting up the server, the pebble engine would be
initialized with Pebble's default logger. The reason for this is that
the pebble initialization code calls `EnsureDefaults` on the
configuration options _before_ checking if the `options.Logger` is
nil/unset. At that point, it will never be unset, as `EnsureDefaults`
will set the logger to `pebble.DefaultLogger` if it was not previously
set.

This change overwrites the pebble logger if its found to be the
`DefaultLogger`. We never want to use pebble's `DefaultLogger` in CRDB
as that would mean pebble would use the standard library `log`
package, making every message emitted by Pebble to be treated as
`INFO` level messages, regardless of severity (including `log.Fatal`
calls).

Related to #83079.

Fixes #72683.
Fixes #90483.

Release note: None.
@blathers-crl blathers-crl bot requested a review from a team as a code owner October 25, 2022 20:23
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-22.2-90478 branch from d534655 to 3cae346 Compare October 25, 2022 20:23
@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 25, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Oct 25, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

:lgtm:

thank you!

@renatolabs renatolabs merged commit 0f42966 into release-22.2 Oct 26, 2022
@renatolabs renatolabs deleted the blathers/backport-release-22.2-90478 branch October 26, 2022 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants