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
