mynameborat commented on a change in pull request #1422:
URL: https://github.com/apache/samza/pull/1422#discussion_r478678631
##########
File path:
samza-yarn/src/main/java/org/apache/samza/job/yarn/YarnClusterResourceManager.java
##########
@@ -485,7 +485,15 @@ public void onContainersAllocated(List<Container>
containers) {
//nodes being updated. We always return 0 when asked for progress by Yarn.
@Override
public void onShutdownRequest() {
- //not implemented currently.
+ log.info("Stopping the AM client on shutdown request.");
+ lifecycle.onShutdown(SamzaApplicationState.SamzaAppStatus.FAILED);
+ amClient.stop();
+ log.info("Stopping the NM client on shutdown request.");
+ nmClientAsync.stop();
+ log.info("Stopping the SamzaYarnAppMasterService service on shutdown
request.");
+ service.onShutdown();
+ log.info("Stopping SamzaAppMasterMetrics on shutdown request.");
+ metrics.stop();
Review comment:
why not leverage `stop(SamzaAppStatus.FAILED)` up top? I like some
additional logs. so maybe modify the `stop(...)` to include this logs for
better debuggability.
----------------------------------------------------------------
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]
