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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
