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

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

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


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -845,8 +877,6 @@ public void onContainersAllocated(List<Container> 
containers) {
               instanceName = null;
             }
           }
-          allocatedContainerCountMap.put(containerHelixTag,

Review Comment:
   Putting this block inside of the synchronized block works is kind of 
confusing. I am not entirely convinced it does what we want it to. I think it 
would lock on the callbackhandler which does prevent concurrent modifications 
if the caller is using this method but not others (see onContainerComplete 
which has no safeguards)
   
   With this being a relatively cold part of the code (once every 90 seconds?), 
we really don't need any fine-grained locking. An atomic integer would be 
sufficient or even a global lock for all instances of the class. (YarnService 
is a single thread and then there is a callback for each container allocated. 
Container allocation / deallocation doesn't happen that much especially after 
the pipeline is up for a few hours. 





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

    Worklog Id:     (was: 818951)
    Time Spent: 4h  (was: 3h 50m)

> 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: 4h
>  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