Skip to content

[improve][broker] Change limitStatsLogging config default value to true#20409

Merged
Technoboy- merged 1 commit into
apache:masterfrom
TakaHiR07:change_limitStatsLogging_default_true
May 31, 2023
Merged

[improve][broker] Change limitStatsLogging config default value to true#20409
Technoboy- merged 1 commit into
apache:masterfrom
TakaHiR07:change_limitStatsLogging_default_true

Conversation

@TakaHiR07

@TakaHiR07 TakaHiR07 commented May 26, 2023

Copy link
Copy Markdown
Contributor

Motivation

Since apache/bookkeeper#3719 has set limitStatsLogging config default true in bookie, it is better to also set it default true in broker config.

Verifying this change

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 threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: TakaHiR07#8

@github-actions github-actions Bot added the doc-complete Your PR changes impact docs and the related docs have been already added. label May 26, 2023
@Technoboy- Technoboy- requested a review from hangc0276 May 29, 2023 14:04

@hangc0276 hangc0276 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't expose the bookie client metrics to Prometheus and exposing the bookie client channel level metrics wast a lot of resources. We can disable the channel level metric by default.

@hangc0276 hangc0276 added this to the 3.1.0 milestone May 30, 2023
@Technoboy- Technoboy- changed the title [improve] change limitStatsLogging config default value to true [improve][broker] Change limitStatsLogging config default value to true May 30, 2023
@Technoboy-

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

@Technoboy- Technoboy- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We only need to keep this change in the master branch, no need to cherry pick to other branches, right ? @hangc0276

@codecov-commenter

codecov-commenter commented May 30, 2023

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.93%. Comparing base (f0e97f4) to head (9a22575).
⚠️ Report is 2491 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20409      +/-   ##
============================================
+ Coverage     72.45%   72.93%   +0.48%     
+ Complexity    31898    31888      -10     
============================================
  Files          1852     1864      +12     
  Lines        138227   138416     +189     
  Branches      15175    15188      +13     
============================================
+ Hits         100150   100955     +805     
+ Misses        30098    29450     -648     
- Partials       7979     8011      +32     
Flag Coverage Δ
inttests 24.15% <100.00%> (-0.03%) ⬇️
systests 24.93% <100.00%> (?)
unittests 72.21% <100.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 99.37% <100.00%> (ø)

... and 153 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Technoboy- Technoboy- merged commit f920117 into apache:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc-complete Your PR changes impact docs and the related docs have been already added.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants