[ 
https://issues.apache.org/jira/browse/GOBBLIN-1728?focusedWorklogId=818550&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-818550
 ]

ASF GitHub Bot logged work on GOBBLIN-1728:
-------------------------------------------

                Author: ASF GitHub Bot
            Created on: 19/Oct/22 17:39
            Start Date: 19/Oct/22 17:39
    Worklog Time Spent: 10m 
      Work Description: homatthew commented on code in PR #3586:
URL: https://github.com/apache/gobblin/pull/3586#discussion_r999761558


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -474,11 +477,21 @@ public synchronized void 
requestTargetNumberOfContainers(YarnContainerRequestBun
     for (Map.Entry<String, Integer> entry : 
yarnContainerRequestBundle.getHelixTagContainerCountMap().entrySet()) {
       String currentHelixTag = entry.getKey();
       int desiredContainerCount = entry.getValue();
+      Resource resourceForHelixTag = 
yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag);
+
       // Calculate requested container count based on adding allocated count 
and outstanding ContainerRequests in Yarn
-      int requestedContainerCount = 
allocatedContainerCountMap.getOrDefault(currentHelixTag, 0)
-          + 
getMatchingRequestsCount(yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag));
-      for(; requestedContainerCount < desiredContainerCount; 
requestedContainerCount++) {
-        requestContainer(Optional.absent(), 
yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag));
+      allocatedContainerCountMap.putIfAbsent(currentHelixTag, new 
AtomicInteger(0));

Review Comment:
   Most of the rest of the changes are logging / code smell changes. 



##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -474,11 +477,21 @@ public synchronized void 
requestTargetNumberOfContainers(YarnContainerRequestBun
     for (Map.Entry<String, Integer> entry : 
yarnContainerRequestBundle.getHelixTagContainerCountMap().entrySet()) {
       String currentHelixTag = entry.getKey();
       int desiredContainerCount = entry.getValue();
+      Resource resourceForHelixTag = 
yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag);
+
       // Calculate requested container count based on adding allocated count 
and outstanding ContainerRequests in Yarn
-      int requestedContainerCount = 
allocatedContainerCountMap.getOrDefault(currentHelixTag, 0)
-          + 
getMatchingRequestsCount(yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag));
-      for(; requestedContainerCount < desiredContainerCount; 
requestedContainerCount++) {
-        requestContainer(Optional.absent(), 
yarnContainerRequestBundle.getHelixTagResourceMap().get(currentHelixTag));
+      allocatedContainerCountMap.putIfAbsent(currentHelixTag, new 
AtomicInteger(0));

Review Comment:
   The main logical change here is changing the value type from `Integer` to 
`AtomicInteger`. This prevents potential desync between our 
allocatedContainerCountMap (HelixTag -> numCountainers) and the containerMap 
(containerId -> Container POJO) due to race condition.
   
   I suspect the incorrect value for allocated container map is one of the 
reasons we saw incorrect behavior with shrink + allocating. This edge case 
would be generally rare and that's why we only start to see issues after a 
large number of days and the issue is resolved after a restart. 





Issue Time Tracking
-------------------

    Worklog Id:     (was: 818550)
    Time Spent: 1h 20m  (was: 1h 10m)

> Yarn Service requests too many containers due to improper calculation
> ---------------------------------------------------------------------
>
>                 Key: GOBBLIN-1728
>                 URL: https://issues.apache.org/jira/browse/GOBBLIN-1728
>             Project: Apache Gobblin
>          Issue Type: New Feature
>            Reporter: Matthew Ho
>            Priority: Major
>          Time Spent: 1h 20m
>  Remaining Estimate: 0h
>
> Yarn Service is responsible for calculating the number of instances based on 
> the helix tasks. Yarn service tracks the number of instances by asking Yarn 
> for the number of resource requests and the number of allocated containers.
>  
> It uses this count to determine if it should ask for more containers or 
> shrink the number of containers. This calculation is currently done 
> improperly and we continue to request containers when we have enough 
> requested.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to