[GitHub] [gobblin] sv2000 commented on a diff in pull request #3519: Make Yarn container and helix instance allocation group by tag

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

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


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -431,27 +427,35 @@ private EventSubmitter buildEventSubmitter() {
    * number of containers. The intended usage is for the caller of this method to make periodic calls to attempt to
    * adjust the cluster towards the desired number of containers.
    *
-   * @param numTargetContainers the desired number of containers
+   * @param yarnContainerRequestBundle the desired containers information, including numbers, resource and helix tag
    * @param inUseInstances  a set of in use instances
    */
-  public synchronized void requestTargetNumberOfContainers(int numTargetContainers, Set<String> inUseInstances) {
-    LOGGER.debug("Requesting numTargetContainers {} current numRequestedContainers {} in use instances {} map size {}",
-        numTargetContainers, this.numRequestedContainers, inUseInstances, this.containerMap.size());
-
+  public synchronized void requestTargetNumberOfContainers(YarnContainerRequestBundle yarnContainerRequestBundle, Set<String> inUseInstances) {
+    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);
-
     // Request additional containers if the desired count is higher than the max of the current allocation or previously
     // requested amount. Note that there may be in-flight or additional allocations after numContainers has been computed
     // so overshooting can occur, but periodic calls to this method will make adjustments towards the target.
-    for (int i = numContainers; i < numTargetContainers; i++) {
-      requestContainer(Optional.<String>absent());
+    for (Map.Entry<String, Integer> entry : yarnContainerRequestBundle.getHelixTagContainerCountMap().entrySet()) {
+      String currentHelixTag = entry.getKey();
+      int desiredContainerCount = entry.getValue();
+      int requestedContainerCount = requestedContainerCountMap.getOrDefault(currentHelixTag, 0);
+      for(; requestedContainerCount < desiredContainerCount; requestedContainerCount++) {
+        requestContainer(Optional.absent(), yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag));
+      }
+      requestedContainerCountMap.put(currentHelixTag, desiredContainerCount);
+    }
+
+    // If a requested tag is not presented in the new request, update the requested count to 0 as we should release them

Review Comment:
   Don't understand the comment. Are you suggesting setting the requested count to 0 to release containers? If so, why is that needed?



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