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