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

Fix performance issue caused by invalid logging configuration #8908

Merged
merged 1 commit into from Dec 11, 2020

Conversation

@lhotari
Copy link
Contributor

@lhotari lhotari commented Dec 11, 2020

Motivation

Profiling a 3 node Pulsar cluster running with 2.7.0 version showed allocation hotspots in log.debug methods. This indicated that the root logger level was set to debug.

Modifications

  • Configure the root log level in bin/pulsar startup script to
    use info level by default. This can be done by setting
    the pulsar.log.root.level system property.

  • since the root log level was debug, all code blocks within
    log.isDebugEnabled() got executed.
    This caused a lot of unnecessary memory allocations and wasted CPU cycles.

- Configure the root log level in bin/pulsar startup script to
  use "info" level by default. This can be done by setting
  the "pulsar.log.root.level" system property.

- since the root log level was debug, all code blocks within
  log.isDebugEnabled() got executed.
  This caused a lot of unnecessary memory allocations and wasted CPU cycles.
@lhotari
Copy link
Contributor Author

@lhotari lhotari commented Dec 11, 2020

Here's an allocation flamegraph for ManagedLedgerImpl.asyncReadEntries method.

Pulsar 2.7.0, a lot of logging calls are present:

image

After applying this fix, the logging calls are gone:

image

@lhotari
Copy link
Contributor Author

@lhotari lhotari commented Dec 11, 2020

/pulsarbot run-failure-checks

@merlimat merlimat added this to the 2.8.0 milestone Dec 11, 2020
@sijie
sijie approved these changes Dec 11, 2020
@lhotari
Copy link
Contributor Author

@lhotari lhotari commented Dec 11, 2020

/pulsarbot run-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Good catch!

+1

@sijie sijie merged commit 18f539e into apache:master Dec 11, 2020
23 checks passed
23 checks passed
build
Details
cpp-tests
Details
backwards-compatibility
Details
cli
Details
function-state
Details
messaging
Details
process
Details
schema
Details
sql
Details
standalone
Details
thread
Details
tiered-filesystem
Details
tiered-jcloud
Details
transaction
Details
License check
Details
shade-check
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.