Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[improve][broker] PIP-192: Add large topic count filter #19613

Conversation

Demogorgon314
Copy link
Member

Master Issue: #16691

Motivation

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

Modifications

This PR added a filter that can filter brokers based on the number of topics.

TODO

The broker load data might be delayed, so the max topic check might not be accurate.

Documentation

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

@Demogorgon314 Demogorgon314 self-assigned this Feb 23, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 23, 2023
@@ -77,12 +79,18 @@ public BrokerLoadData generateLoadData() {
final var pulsarStats = pulsar.getBrokerService().getPulsarStats();
synchronized (pulsarStats) {
var brokerStats = pulsarStats.getBrokerStats();
var bundleStats = pulsarStats.getBundleStats();
var topics = 0;
for (Map.Entry<String, NamespaceBundleStats> bundleStatsEntry : bundleStats.entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define topics in BrokerStats and pre-aggregate this topic counts in updateStats?


   public synchronized void updateStats(
            ConcurrentOpenHashMap<String, ConcurrentOpenHashMap<String, ConcurrentOpenHashMap<String, Topic>>>
                    topicsMap) {
...
                    bundles.forEach((bundle, topics) -> {
                        NamespaceBundleStats currentBundleStats = bundleStats.computeIfAbsent(bundle,
                                k -> new NamespaceBundleStats());
                        currentBundleStats.reset();
                        currentBundleStats.topics = topics.size();
                        brokerStats.topics += topics.size(); //pre-aggregate

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, pre-aggregate will be better. Updated.

Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM, maybe we can name the filter as MaxTopicCountFilter or BrokerMaxTopicCountFilter?

@Technoboy- Technoboy- added this to the 3.0.0 milestone Mar 2, 2023
@Technoboy- Technoboy- closed this Mar 2, 2023
@Technoboy- Technoboy- reopened this Mar 2, 2023
@Demogorgon314 Demogorgon314 requested a review from gaoran10 March 2, 2023 03:12
@Technoboy- Technoboy- added type/feature The PR added a new feature or issue requested a new feature area/broker labels Mar 2, 2023
@Technoboy- Technoboy- merged commit fec4578 into apache:master Mar 2, 2023
@Demogorgon314 Demogorgon314 deleted the Demogorgon314/PIP-192-Add_LargeTopicCountFilter branch March 3, 2023 01:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants