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

[Branch-2.9][Cherry-pick] fix bug: fail to expose managed ledger client stats to prometheus if bookkeeperClientExposeStatsToPrometheus is true #16343

Merged
merged 1 commit into from
Jul 2, 2022

Conversation

HQebupt
Copy link
Contributor

@HQebupt HQebupt commented Jul 2, 2022

Cherry-pick #16219

Motivation

It failed to expose bookkeeper client managed ledger stats to prometheus if setting bookkeeperClientExposeStatsToPrometheus=true in ServiceConfiguration. Because creating BkClient ignore the StatsLogger and use the NullStatsLogger.INSTANCE as default.

//class: org.apache.pulsar.broker.BookKeeperClientFactoryImpl
    @Override
    public BookKeeper create(ServiceConfiguration conf, ZooKeeper zkClient, EventLoopGroup eventLoopGroup,
                             Optional<Class<? extends EnsemblePlacementPolicy>> ensemblePlacementPolicyClass,
                             Map<String, Object> properties) throws IOException {
        return create(conf, zkClient, eventLoopGroup, ensemblePlacementPolicyClass, properties,
                NullStatsLogger.INSTANCE);
    }

Modifications

Add StatsLogger into BookKeeperClientFactory when creating BkClient.

The screenshot below is the result of my verification.
image

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 yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc-not-needed

…nt stats to prometheus if bookkeeperClientExposeStatsToPrometheus is true apache#16219
@HQebupt HQebupt force-pushed the cherry-pick-16219 branch from 311d3ab to e85c915 Compare July 2, 2022 06:58
@mattisonchao mattisonchao reopened this Jul 2, 2022
@mattisonchao
Copy link
Member

Re-open this PR to rebase the branch.

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 2, 2022

/pulsarbot run-failure-checks

@mattisonchao mattisonchao merged commit d56ae37 into apache:branch-2.9 Jul 2, 2022
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jul 2, 2022
BewareMyPower pushed a commit that referenced this pull request Jul 29, 2022
…nt stats to prometheus if bookkeeperClientExposeStatsToPrometheus is true #16219 (#16343)

(cherry picked from commit d56ae37)
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.8.4 release/2.9.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants