[GitHub] [gobblin] phet commented on a diff in pull request #3499: [GOBBLIN-1638] Fix unbalanced running count metrics due to Azkaban failures

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

phet commented on code in PR #3499:
URL: https://github.com/apache/gobblin/pull/3499#discussion_r863038637


##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/UserQuotaManagerTest.java:
##########
@@ -68,4 +67,17 @@ public void testExceedsQuotaThrowsException() throws Exception {
       this._quotaManager.checkQuota(dags.get(1).getNodes().get(0), false);
     });
   }
+
+  @Test
+  public void testMultipleRemoveQuotasNegative() throws Exception {

Review Comment:
   technically aren't you testing that multiple (specifically, repeated) removes *don't* leave the quota negative.  method name suggests that you're testing that they do go negative.
   
   minor: you aren't truly verifying whether a value is pos/neg, but just that repeated removes are ignored.  that may be fine, but if it's really what you need to ensure, it might be best to look at it specifically.



##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -952,6 +952,10 @@ private void submitJob(DagNode<JobExecutionPlan> dagNode) {
         TimingEvent jobOrchestrationTimer = this.eventSubmitter.isPresent() ? this.eventSubmitter.get().
             getTimingEvent(TimingEvent.LauncherTimings.JOB_ORCHESTRATED) : null;
 
+        if (this.metricContext != null) {
+          getRunningJobsCounter(dagNode).inc();
+          getRunningJobsCounterForUser(dagNode).forEach(ContextAwareCounter::inc);

Review Comment:
   this may require a comment (at least for me to grok)... seems you are incrementing counters before actually submitting the job for execution, correct? (even prior to this change we didn't wait to find for the future's result)
   
   the async future does present a challenge (since we don't await fulfillment).  should perhaps the increment happen within the future, once the result of submission is actually know?
   
   I could imagine an argument for using the increment as a sort of pre-reservation (locking) to avoid exceeding the allotment.  therefore doing just prior to submission.  but, that said, I thought quota management (`checkQuota`) does just that, so wouldn't be necessary to duplicate.
   
   anyway... I may be confused... just let me know your strategy here--and consider a comment so I (and other maintainers) will more readily understand.



-- 
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