MYRIAD-250 Should shutdown mesos framework when stop resourcemanager #101
Conversation
| @@ -66,6 +66,10 @@ public void init(Configuration conf, AbstractYarnScheduler yarnScheduler, RMCont | |||
| } | |||
|
|
|||
| @Override | |||
| public void cleanup() throws IOException { | |||
| } | |||
There was a problem hiding this comment.
Am I missing something or do no interceptors actually throw IOException?
| LOGGER.info("stopping mesosDriver.."); | ||
| Main.getInjector().getInstance(MyriadDriverManager.class).stopDriver(false); | ||
| LOGGER.info("stopped mesosDriver.."); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Nit: can you the specific exceptions?
There was a problem hiding this comment.
Sorry for not handling exceptions carefully earlier.
Actually here stopDriver would not throw exceptions. It seems to me that error would reflect to the return value of stopDriver.
The same with MyriadInitializationInterceptor does in init(), in my opinion, methods such as init, cleanup in interface should always have to throw out exceptions no matter whether the implements actually do.
And I prefer here throws IOExceptions rather than RuntimeException(actually the behavior has no difference)
|
Could you please add unittest? |
|
I have tested this PR and it is correct. I have done some unit tests as proposed by @yufeldman. If you consider merging, I do a PR with the tests. |
| * Responsible for shutdown Myriad by invoking stopDriver upon the | ||
| * Myriad driver {@link org.apache.myriad.Main} | ||
| */ | ||
| public class MyriadCleanupInterceptor extends BaseInterceptor { |
There was a problem hiding this comment.
Do we really need cleanup interceptor or it can be part of MyriadInitializationInterceptor - cleanup() method?
There was a problem hiding this comment.
Hi @yufeldman, it seems that @taojieterry does not respond. In my opinion you do not need a new interceptor, you can take advantage of the existing one. Perhaps with a name change like MyriadHelperInterceptor or MyriadStartStopInterceptor so that the behavior of the class is clear?
|
@jpgilaberte I still don't see any unit tests. You said you tested, but I guess this was just manual testing. |
|
Hello and thanks @yufeldman. I have tested the PR and generated some unit tests but I do not know how to include the tests in this PR. I propose merge this PR, and them I can create a new PR adding the unittests for this case. If the procedure is different, I do not know it. I would appreciate it if you would indicate what is the procedure. Thanks. |
Added MyriadCleanupInterceptor, once scheduler(fair, fifo, capacity) is stopped,
MyriadDriverManager.stopDriver()will be invoked to shutdown the mesos framework.As a result, all nodemanagers launched by the resourcemanger would be killed once the RM is stopped.