[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3511: [GOBBLIN-1650] Implement flowGroup quotas for the DagManager

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

Will-Lo commented on code in PR #3511:
URL: https://github.com/apache/gobblin/pull/3511#discussion_r880993561


##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/UserQuotaManager.java:
##########
@@ -159,6 +179,9 @@ public boolean releaseQuota(Dag.DagNode<JobExecutionPlan> dagNode) {
       String proxyUserKey = DagManagerUtils.getUserQuotaKey(proxyUser, dagNode);
       decrementQuotaUsage(proxyUserToJobCount, proxyUserKey);
     }
+    String flowGroup = ConfigUtils.getString(dagNode.getValue().getJobSpec().getConfig(),
+        ConfigurationKeys.FLOW_GROUP_KEY, "");
+    decrementQuotaUsage(flowGroupToJobCount, DagManagerUtils.getFlowGroupQuotaKey(flowGroup, dagNode));
     String serializedRequesters = DagManagerUtils.getSerializedRequesterList(dagNode);
     if (serializedRequesters != null) {

Review Comment:
   I have a test from a previous PR in`DagManagerTest` that also tested for releaseQuota API in terms of the larger flow lifecycle.
   
   `releaseQuota` is only called when a flow is completed so besides that the only scenario where the count decrements is when an increment fails due to exceeding the quota, but this is tested in the below test  `testUserAndFlowGroupQuotaMultipleUsersAdd` since a failed increment is followed with a decrement then increment



-- 
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: dev-unsubscribe@gobblin.apache.org

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