[GitHub] [pulsar] eolivelli commented on a change in pull request #11331: [PIP-82] [pulsar-broker] updates to resource-group stats:

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #11331:
URL: https://github.com/apache/pulsar/pull/11331#discussion_r671308616



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroup.java
##########
@@ -294,13 +318,15 @@ protected BytesAndMessagesCount getGlobalUsageStats(ResourceGroupMonitoringClass
     protected BytesAndMessagesCount updateLocalQuota(ResourceGroupMonitoringClass monClass,
                                                      BytesAndMessagesCount newQuota) throws PulsarAdminException {
         this.checkMonitoringClass(monClass);
-        BytesAndMessagesCount oldBMCount = new BytesAndMessagesCount();
+        BytesAndMessagesCount oldBMCount;
 
         final PerMonitoringClassFields monEntity = this.monitoringClassFields[monClass.ordinal()];
         monEntity.localUsageStatsLock.lock();
         oldBMCount = monEntity.quotaForNextPeriod;
         try {
             monEntity.quotaForNextPeriod = newQuota;
+            this.resourceGroupPublishLimiter.publishMaxByteRate = newQuota.bytes;
+            this.resourceGroupPublishLimiter.publishMaxMessageRate = (int) newQuota.messages;

Review comment:
       is this cast dangerous ? 
   
   can we add a method to  `resourceGroupPublishLimiter` in order to not access directly the fields (better encapsulation) ?

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroup.java
##########
@@ -310,6 +336,21 @@ protected BytesAndMessagesCount updateLocalQuota(ResourceGroupMonitoringClass mo
         return oldBMCount;
     }
 
+    protected BytesAndMessagesCount getRgPublishRateLimiterValues() {
+        BytesAndMessagesCount retVal = new BytesAndMessagesCount();
+        final PerMonitoringClassFields monEntity =
+                                        this.monitoringClassFields[ResourceGroupMonitoringClass.Publish.ordinal()];
+        monEntity.localUsageStatsLock.lock();
+        try {
+            retVal.bytes = this.resourceGroupPublishLimiter.publishMaxByteRate;

Review comment:
       we can have a method that returns a BytesAndMessagesCount directly in `resourceGroupPublishLimiter`, this way we are not accessing directly the fields

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupService.java
##########
@@ -672,8 +690,10 @@ private void checkRGCreateParams(String rgName, org.apache.pulsar.common.policie
     private ConcurrentHashMap<String, ResourceGroup> namespaceToRGsMap = new ConcurrentHashMap<>();
 
     // Maps to maintain the usage per topic, in produce/consume directions.
-    private ConcurrentHashMap<String, BytesAndMessagesCount> topicProduceStats = new ConcurrentHashMap();
-    private ConcurrentHashMap<String, BytesAndMessagesCount> topicConsumeStats = new ConcurrentHashMap();
+    private ConcurrentHashMap<String, BytesAndMessagesCount> topicProduceStats =
+                                                new ConcurrentHashMap<String, BytesAndMessagesCount>();

Review comment:
       nit: you can use '<>' instead of `<String, BytesAndMessagesCount>`




-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org