[
https://issues.apache.org/jira/browse/YARN-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15477938#comment-15477938
]
Arun Suresh edited comment on YARN-5620 at 9/9/16 6:50 PM:
-----------------------------------------------------------
[~jianhe], thanks for thoughtful comments.
bq. Also, if it’s ignored here, then it appears to AM that the upgrade call is
somehow ignored
So in the {{ReInitializeContainerTransition}}, I do the following
{noformat}
if (container.reInitContext != null) {
container.addDiagnostics("Container [" + container.getContainerId()
+ "] ReInitialization already in progress !!");
return;
}
{noformat}
So a new upgrade is not ignored..
bq. I think we can move the logic of ReInitializeContainerTransition to the
upgrade API ? All it does is to resend the events to containerLauncher or
ResourceLocalizationService , which can be done in the API. Also, this has the
benefit of rejecting the upgrade call while the previous upgrade is
in_progress.
Actually, in one of my initial versions, I was setting the reInitContext on the
container object in the upgrade API itself. But then I realized that all state
change on the Container is affected via a state machine transition and I did
not want to break that. I think we should keep the state change of a Container
limited to state machine transitions as the code is cleaner and we can guard
against other synchronization issues.
Also, I think we should have a consistent way to letting the AM know about
errors. I prefer we use the diagnostic messages (with maybe an error code) to
let the AM know if failures. I guess you are already doing it for Failed
Localization. This way, the checks and enforcements are done only in
transitions. I agree the AM will not know immediately of an error, but
subsequent 'getContainerStatus' calls should notify the AM.
bq. One other potential race is that the relocalize API has a chance to go
through instead of rejected, while upgrading,
I understand.. I was actually thinking we have a set of
*reInitilizingContainerIds* in the NM context. we can check if containerId is
present in the set in the upgrade and relocalize API and add to the set if not.
We then remove from this set once the Container moves from LOCALIZED to RUNNING
(this signifies that reinitialization is complete). Thoughts ?
bq. Given so many if(reinitializing) conditions in containerImpl, should we
consider adding a new state?
I tried that at first... but I preferred having more if thens than extra
Container states. I can provide a version with different container states
though...
bq. when launching the container, we need to cleanupPreviousContainerFiles as
done in ContainerRelaunch, right?
This is actually something I wanted to discuss. Do you think we should cleanup
the old script file ? If the upgrade uses the same script file name, it will
be overwritten right ? the token file is anyway overwritten right ?
was (Author: asuresh):
[~jianhe], thanks for thoughtful comments.
bq. Also, if it’s ignored here, then it appears to AM that the upgrade call is
somehow ignored
So in the {{ReInitializeContainerTransition}}, I do the following
{noformat}
if (container.reInitContext != null) {
container.addDiagnostics("Container [" + container.getContainerId()
+ "] ReInitialization already in progress !!");
return;
}
{noformat}
So a new upgrade is not ignored..
bq. I think we can move the logic of ReInitializeContainerTransition to the
upgrade API ? All it does is to resend the events to containerLauncher or
ResourceLocalizationService , which can be done in the API. Also, this has the
benefit of rejecting the upgrade call while the previous upgrade is
in_progress.
Actually, in one of my initial versions, I was setting the reInitContext on the
container object in the upgrade API itself. But then I realized that all state
change on the Container is affected via a state machine transition and I did
not want to break that. I think we should keep the state change of a Container
limited to state machine transitions as the code is cleaner and we can guard
against other synchronization issues.
Also, I think we should have a consistent way to letting the AM know about
errors. I prefer we use the diagnostic messages (with maybe an error code) to
let the AM know if failures. I guess you are already doing it for Failed
Localization. This way, the checks and enforcements are done only in
transitions. I agree the AM will not know immediately of an error, but
subsequent 'getContainerStatus' calls should notify the AM.
bq. One other potential race is that the relocalize API has a chance to go
through instead of rejected, while upgrading,
I understand.. I was actually thinking we have a set of
*reInitilizingContainerId* in the NM context. we can check if containerId is
present in the set before performing upgrade and relocalize and we can remove
from this set once the Container moves from LOCALIZED to RUNNING (this
signifies that reinitialization is complete)
bq. Given so many if(reinitializing) conditions in containerImpl, should we
consider adding a new state?
I tried that at first... but I preferred having more if thens than extra
Container states. I can provide a version with different container states
though...
bq. when launching the container, we need to cleanupPreviousContainerFiles as
done in ContainerRelaunch, right?
This is actually something I wanted to discuss. Do you think we should cleanup
the old script file ? If the upgrade uses the same script file name, it will
be overwritten right ? the token file is anyway overwritten right ?
> Core changes in NodeManager to support for upgrade and rollback of Containers
> -----------------------------------------------------------------------------
>
> Key: YARN-5620
> URL: https://issues.apache.org/jira/browse/YARN-5620
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Arun Suresh
> Assignee: Arun Suresh
> Attachments: YARN-5620.001.patch, YARN-5620.002.patch,
> YARN-5620.003.patch, YARN-5620.004.patch, YARN-5620.005.patch,
> YARN-5620.006.patch, YARN-5620.007.patch
>
>
> JIRA proposes to modify the ContainerManager (and other core classes) to
> support upgrade of a running container with a new {{ContainerLaunchContext}}
> as well as the ability to rollback the upgrade if the container is not able
> to restart using the new launch Context.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
