michaeljmarshall commented on code in PR #16932:
URL: https://github.com/apache/pulsar/pull/16932#discussion_r944093002
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java:
##########
@@ -748,6 +749,18 @@ public void updateRates(NamespaceStats nsStats,
NamespaceBundleStats bundleStats
topicStatsStream.writePair("msgThroughputOut",
subMsgThroughputOut);
topicStatsStream.writePair("msgRateRedeliver",
subMsgRateRedeliver);
topicStatsStream.writePair("type",
subscription.getTypeString());
+
+ // Write entry filter stats
+ Dispatcher dispatcher0 = subscription.getDispatcher();
+ topicStatsStream.writePair("entryFilterProccessedMsgs",
+ dispatcher0 == null ? 0 :
dispatcher0.getFilterProcessesMsgsCount());
+ topicStatsStream.writePair("entryFilterAcceptedMsgs",
+ dispatcher0 == null ? 0 :
dispatcher0.getFilterAcceptedMsgsCount());
+ topicStatsStream.writePair("entryFilterRejectedMsgs",
+ dispatcher0 == null ? 0 :
dispatcher0.getFilterRejectedMsgsCount());
+ topicStatsStream.writePair("entryFilterRescheduledMsgs",
+ dispatcher0 == null ? 0 :
dispatcher0.getFilterRescheduledMsgsCount());
Review Comment:
Nit: it seems like we could consolidate this logic with the `if
(subscription.getDispatcher() != null)` block below to minimize the clutter of
the `null` checks for the `dispatcher`.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/AggregatedSubscriptionStats.java:
##########
@@ -64,5 +64,13 @@ public class AggregatedSubscriptionStats {
long consumersCount;
+ long throughFilterMsgCount;
Review Comment:
Question about this term. It maps to the metric
`pulsar_subscription_filter_processed_msg_count`. In order to make it really
clear that the mapping is correct, I think it might make sense to use
`filterProcessedMsgCount` for this variable. What do you think?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Dispatcher.java:
##########
@@ -129,4 +129,21 @@ default boolean checkAndUnblockIfStuck() {
return false;
}
+
+ default long getFilterProcessesMsgsCount() {
Review Comment:
Nit: it is `Processed` elsewhere. I think it should be `Processed` here too,
given that all of the other fields and methods are using the past tense.
```suggestion
default long getFilterProcessedMsgsCount() {
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Dispatcher.java:
##########
@@ -129,4 +129,21 @@ default boolean checkAndUnblockIfStuck() {
return false;
}
+
+ default long getFilterProcessesMsgsCount() {
+ return 0;
+ }
+
+ default long getFilterAcceptedMsgsCount() {
+ return 0;
+ }
+
+ default long getFilterRejectedMsgsCount() {
+ return 0;
+ }
+
+ default long getFilterRescheduledMsgsCount() {
+ return 0;
+ }
Review Comment:
This is pretty minor, but for each of these, I think we might want to
consider using `Msg` instead of `Msgs`. There are many usages of `MsgCount` in
the project, but none of `MsgsCount`. If you feel strongly, you can ignore this
comment.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
