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] PIP-192 Added broker and top-bundles load reporters #19471

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

heesung-sn
Copy link
Contributor

Master Issue: #16691

Motivation

We will start raising PRs to implement PIP-192, #16691

Modifications

This PR added broker and .top-bundles load data reporters.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • *Added unit tests.

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

Documentation

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

We will have separate PRs to update the Doc later.

Matching PR in forked repository

PR in forked repository: heesung-sn#29

}
} catch (Throwable e) {
log.error("Failed to report top-bundles load data.", e);
return CompletableFuture.failedFuture(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to catch exception ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to wrap any unexpected exception and continue the next run on the scheduler thread.

Copy link
Contributor

@Technoboy- Technoboy- Feb 9, 2023

Choose a reason for hiding this comment

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

yes, but seems no error from generateLoadData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this try-catch, and added try-catch to the scheduler.

@heesung-sn heesung-sn force-pushed the pip-192-load-reporters branch 2 times, most recently from b096091 to 2201a18 Compare February 9, 2023 19:18
@heesung-sn heesung-sn force-pushed the pip-192-load-reporters branch from f85557f to 35829b3 Compare February 10, 2023 22:23
@Technoboy- Technoboy- force-pushed the pip-192-load-reporters branch from 35829b3 to 3dcea01 Compare February 12, 2023 13:30
@Technoboy- Technoboy- closed this Feb 12, 2023
@Technoboy- Technoboy- reopened this Feb 12, 2023
@heesung-sn heesung-sn force-pushed the pip-192-load-reporters branch 2 times, most recently from 2f6c28d to 76d004d Compare February 12, 2023 20:26
@heesung-sn heesung-sn force-pushed the pip-192-load-reporters branch from 76d004d to 3879428 Compare February 12, 2023 20:45
@Technoboy- Technoboy- added this to the 3.0.0 milestone Feb 13, 2023
@Technoboy- Technoboy- closed this Feb 13, 2023
@Technoboy- Technoboy- reopened this Feb 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023

Codecov Report

Attention: Patch coverage is 9.77778% with 203 lines in your changes missing coverage. Please review.

Project coverage is 64.43%. Comparing base (5c8f929) to head (3879428).
Report is 1680 commits behind head on master.

Files with missing lines Patch % Lines
...ce/extensions/reporter/BrokerLoadDataReporter.java 0.00% 64 Missing ⚠️
...ker/loadbalance/extensions/models/TopKBundles.java 0.00% 53 Missing ⚠️
...extensions/reporter/TopBundleLoadDataReporter.java 0.00% 28 Missing ⚠️
...dbalance/extensions/ExtensibleLoadManagerImpl.java 0.00% 23 Missing ⚠️
...dbalance/extensions/scheduler/TransferShedder.java 0.00% 19 Missing ⚠️
...er/loadbalance/extensions/data/BrokerLoadData.java 0.00% 10 Missing ⚠️
...oadbalance/extensions/data/TopBundlesLoadData.java 0.00% 4 Missing ⚠️
.../org/apache/pulsar/broker/service/PulsarStats.java 88.23% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19471      +/-   ##
============================================
+ Coverage     63.30%   64.43%   +1.13%     
- Complexity    26123    26674     +551     
============================================
  Files          1836     1843       +7     
  Lines        134416   135468    +1052     
  Branches      14772    14931     +159     
============================================
+ Hits          85087    87291    +2204     
+ Misses        41649    40336    -1313     
- Partials       7680     7841     +161     
Flag Coverage Δ
inttests 24.86% <9.77%> (-0.03%) ⬇️
systests 25.40% <9.77%> (-0.16%) ⬇️
unittests 61.89% <9.77%> (+1.26%) ⬆️

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 98.72% <100.00%> (+<0.01%) ⬆️
...va/org/apache/pulsar/broker/stats/BrokerStats.java 100.00% <100.00%> (ø)
.../org/apache/pulsar/broker/service/PulsarStats.java 82.30% <88.23%> (+1.77%) ⬆️
...oadbalance/extensions/data/TopBundlesLoadData.java 0.00% <0.00%> (ø)
...er/loadbalance/extensions/data/BrokerLoadData.java 0.00% <0.00%> (ø)
...dbalance/extensions/scheduler/TransferShedder.java 0.00% <0.00%> (ø)
...dbalance/extensions/ExtensibleLoadManagerImpl.java 5.03% <0.00%> (-0.86%) ⬇️
...extensions/reporter/TopBundleLoadDataReporter.java 0.00% <0.00%> (ø)
...ker/loadbalance/extensions/models/TopKBundles.java 0.00% <0.00%> (ø)
...ce/extensions/reporter/BrokerLoadDataReporter.java 0.00% <0.00%> (ø)

... and 197 files with indirect coverage changes

@Demogorgon314 Demogorgon314 merged commit de4f620 into apache:master Feb 13, 2023
@heesung-sn heesung-sn deleted the pip-192-load-reporters branch April 2, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants