heesung-sn commented on code in PR #22518: URL: https://github.com/apache/pulsar/pull/22518#discussion_r1567728500
########## pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java: ########## @@ -2111,6 +2111,10 @@ public void updateRates(NamespaceStats nsStats, NamespaceBundleStats bundleStats bundleStats.producerCount += producers.size(); topicStatsStream.startObject(topic); + if (brokerService.pulsar().getConfig().isEnableReplaceProducerStatsWithTopicStats()) { + rateIn.calculateRate(); Review Comment: It seems like we don't skip/update topicStatsHelper.aggMsgRateIn and aggMsgThroughputIn here. ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java: ########## @@ -157,6 +158,7 @@ public abstract class AbstractTopic implements Topic, TopicPolicyListener<TopicP protected volatile Pair<String, List<EntryFilter>> entryFilters; protected volatile boolean transferring = false; private volatile List<PublishRateLimiter> activeRateLimiters; + protected Rate rateIn = new Rate(); Review Comment: can we rename it to `protected final Rate msgRateIn`? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java: ########## @@ -885,6 +887,13 @@ public void recordAddLatency(long latency, TimeUnit unit) { PUBLISH_LATENCY.observe(latency, unit); } + @Override + public void recordRateIn(long events, long totalValue) { + if (brokerService.pulsar().getConfig().isEnableReplaceProducerStatsWithTopicStats()) { Review Comment: I think we should check this flag before calling this func. ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java: ########## @@ -885,6 +887,13 @@ public void recordAddLatency(long latency, TimeUnit unit) { PUBLISH_LATENCY.observe(latency, unit); } + @Override + public void recordRateIn(long events, long totalValue) { Review Comment: recordMsgRateIn ########## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ########## @@ -3449,6 +3449,13 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece ) private Set<String> additionalServlets = new TreeSet<>(); + @FieldContext( + category = CATEGORY_SERVER, + dynamic = true, + doc = "Enable or disable replace producer stats with topic stats when calculating topic production rate" + ) + private boolean enableReplaceProducerStatsWithTopicStats = false; Review Comment: `precomputeProducerStatsInTopicStats` might be the better name here. ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java: ########## @@ -2391,6 +2395,12 @@ public CompletableFuture<? extends TopicStatsImpl> asyncGetStats(GetStatsOptions } }); + // Replace producer stats with topic-level stats Review Comment: If this flag is enabled, I think we can skip the above stats.msgRateIn and stats.msgThroughputIn computes to optimize. ```java producers.values().forEach(producer -> { PublisherStatsImpl publisherStats = producer.getStats(); if (!isEnableReplaceProducerStatsWithTopicStats ){ stats.msgRateIn += publisherStats.msgRateIn; stats.msgThroughputIn += publisherStats.msgThroughputIn; } if (producer.isRemote()) { remotePublishersStats.put(producer.getRemoteCluster(), publisherStats); } if (!getStatsOptions.isExcludePublishers()){ stats.addPublisher(publisherStats); } }); if (isEnableReplaceProducerStatsWithTopicStats ){ stats.msgRateIn = rateIn.getRate(); stats.msgThroughputIn = rateIn.getValueRate(); } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org