[GitHub] [gobblin] sv2000 commented on a diff in pull request #3504: Fix bug when shrinking the container in Yarn service

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

sv2000 commented on code in PR #3504:
URL: https://github.com/apache/gobblin/pull/3504#discussion_r868567607


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -462,7 +448,7 @@ public synchronized void requestTargetNumberOfContainers(YarnContainerRequestBun
       for(; requestedContainerCount < desiredContainerCount; requestedContainerCount++) {
         requestContainer(Optional.absent(), yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag));
       }
-      requestedContainerCountMap.put(currentHelixTag, requestedContainerCount);
+      requestedContainerCountMap.put(currentHelixTag, desiredContainerCount);

Review Comment:
   I think we discussed this as part of PR#3487: why would desiredContainerCount and requestedContainerCount not be the same here? 



##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -440,17 +431,12 @@ private EventSubmitter buildEventSubmitter() {
    * @param inUseInstances  a set of in use instances
    */
   public synchronized void requestTargetNumberOfContainers(YarnContainerRequestBundle yarnContainerRequestBundle, Set<String> inUseInstances) {
-    LOGGER.debug("Requesting numTargetContainers {}, current numRequestedContainers {} in use, instances {} map size is {}",
-        yarnContainerRequestBundle.getTotalContainers(), this.numRequestedContainers, inUseInstances, this.containerMap.size());
+    LOGGER.debug("Requesting numTargetContainers {}, in use instances count is {}, container map size is {}",
+        yarnContainerRequestBundle.getTotalContainers(), inUseInstances, this.containerMap.size());
     int numTargetContainers = yarnContainerRequestBundle.getTotalContainers();
     // YARN can allocate more than the requested number of containers, compute additional allocations and deallocations
     // based on the max of the requested and actual allocated counts
-    int numAllocatedContainers = this.containerMap.size();
-
-    // The number of allocated containers may be higher than the previously requested amount
-    // and there may be outstanding allocation requests, so the max of both counts is computed here
-    // and used to decide whether to allocate containers.
-    int numContainers = Math.max(numRequestedContainers, numAllocatedContainers);
+    int numAllocatedContainers = Math.max(this.containerMap.size(), allocatedContainerCountMap.values().stream().mapToInt(Integer::intValue).sum());

Review Comment:
   Why would there be a discrepancy between containerMap.size() and allocatedContainerCountMap size? Maybe add a comment explaining why we are introducing a max() calculation.



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