sv2000 commented on a change in pull request #2770: [GOBBLIN-916] Make
ContainerLaunchContext instantiation in YarnService more efficient
URL: https://github.com/apache/incubator-gobblin/pull/2770#discussion_r336316790
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java
##########
@@ -610,6 +610,9 @@ private boolean shouldStickToTheSameNode(int
containerExitStatus) {
*/
private void handleContainerCompletion(ContainerStatus containerStatus) {
Map.Entry<Container, String> completedContainerEntry =
this.containerMap.remove(containerStatus.getContainerId());
+ if(completedContainerEntry == null) {
+ LOGGER.warn(String.format("Container %s already been removed from
containerMap before completed, there may be something wrong when starting the
container", containerStatus.getContainerId()));
+ }
String completedInstanceName = completedContainerEntry.getValue();
Review comment:
We may still get a NPE on line 616 if completedContainerEntry is null. I
think the fix probably should be to not remove the instance from the
containerMap inside NMClientCallbackHandler#onStartContainerError(), since
handleContainerCompletion() should be called even if there is a container
launch error. Also, the double removal is preventing a replacement container
being launched from handleContainerCompletion() method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
