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

[improve][broker] Optimize the log printing of PulsarService #18244

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

Pomelongan
Copy link
Contributor

Motivation

In org.apache.pulsar.broker.PulsarService#start, two log printing can be combined into one line.

LOG.info("messaging service is ready, bootstrap_seconds={}", bootstrapTimeSeconds);
LOG.info("messaging service is ready, {}, cluster={}, configs={}", bootstrapMessage,
config.getClusterName(), config);

Modifications

Combine the two log lines in the org.apache.pulsar.broker.PulsarService#start into one line.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: Pomelongan#11

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 28, 2022
LOG.info("messaging service is ready, bootstrap_seconds={}", bootstrapTimeSeconds);
LOG.info("messaging service is ready, {}, cluster={}, configs={}", bootstrapMessage,
config.getClusterName(), config);
LOG.info("messaging service is ready, {}, cluster={}, configs={}, bootstrap_seconds={}", bootstrapMessage,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better move the bootstrap_seconds to first position, because the configs will be very long

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codelipenghui @AnonHxy I have updated, PTAL.

@AnonHxy AnonHxy changed the title [improve][broker]optimize the log printing logic of PulsarService [improve][broker] Optimize the log printing of PulsarService Oct 29, 2022
@AnonHxy AnonHxy added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Oct 29, 2022
@AnonHxy AnonHxy added this to the 2.12.0 milestone Oct 29, 2022
@Pomelongan Pomelongan force-pushed the update_PulsarService branch from 2ec2dbf to 75e867a Compare October 29, 2022 08:45
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #18244 (75e867a) into master (46c0438) will increase coverage by 10.64%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #18244       +/-   ##
=============================================
+ Coverage     38.84%   49.48%   +10.64%     
+ Complexity     8286     6999     -1287     
=============================================
  Files           683      398      -285     
  Lines         67323    43587    -23736     
  Branches       7215     4475     -2740     
=============================================
- Hits          26152    21570     -4582     
+ Misses        38210    19634    -18576     
+ Partials       2961     2383      -578     
Flag Coverage Δ
unittests 49.48% <100.00%> (+10.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...n/java/org/apache/pulsar/broker/PulsarService.java 57.14% <100.00%> (-0.74%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 3.70% <0.00%> (-77.78%) ⬇️
...lsar/broker/service/StickyKeyConsumerSelector.java 50.00% <0.00%> (-50.00%) ⬇️
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java 0.00% <0.00%> (-40.75%) ⬇️
...e/HashRangeExclusiveStickyKeyConsumerSelector.java 0.00% <0.00%> (-36.93%) ⬇️
...ce/schema/validator/StructSchemaDataValidator.java 52.38% <0.00%> (-9.53%) ⬇️
...sistent/PersistentDispatcherMultipleConsumers.java 51.15% <0.00%> (-8.53%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 37.90% <0.00%> (-5.23%) ⬇️
...r/service/SystemTopicTxnBufferSnapshotService.java 73.91% <0.00%> (-4.35%) ⬇️
... and 319 more

@AnonHxy AnonHxy merged commit f029757 into apache:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants