hanghangliu commented on code in PR #3504:
URL: https://github.com/apache/gobblin/pull/3504#discussion_r868590961
##########
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:
Yes, they should be the same. So using containerMap.size() directly
##########
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 didn't put this case into consideration: in the desiredContainerCount is
smaller than requestedContainerCount, the for-loop wouldn't update the
requestedContainerCount. So need to use desiredContainerCount instead.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
