> On April 13, 2016, 5:11 a.m., Vinod Kone wrote: > > Are you also planning to update slave recovery tests? Those are the most > > crucial. > > Qian Zhang wrote: > Sure, I will update slave recovery tests soon. Just want to double > confirm, in `slave_recovery_tests.cpp`, I see there are two TODOs related to > HTTP based command executor in two test cases: > > https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L462 > > https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L1082 > > You mean I need to update these two test cases with the HTTP command > executor I implemented in MESOS-3558, right? > > Vinod Kone wrote: > Definitely those two and the one we talked about in the earlier review. > > It would also be good to add the following tests for HTTP command > executor. There are already tests for this for driver based command executor. > > --> RecoverUnregisteredHTTPExecutor > --> RecoverTerminatedExecutor > --> KillTask > > Qian Zhang wrote: > OK, I have posted a patch (https://reviews.apache.org/r/46204/) to update > those two tests and add two more (`RecoverUnregisteredHTTPExecutor` and > `KillTaskWithHTTPExecutor`) in slave recovery tests, and I will post a > separate patch for adding the test that slave restarts with flag change. > > But for the test `RecoverTerminatedExecutor`, I have a question: we need > to shutdown the executor when agent is down, for the driver based executor, I > see we use `process::post(executorPid, ShutdownExecutorMessage())`, but for > HTTP based executor, I am not sure how to shutdown it in the test, any > suggestions? Thanks. > > Anand Mazumdar wrote: > We currently don't have the ability to insert `Event`'s on the response > stream. I filed https://issues.apache.org/jira/browse/MESOS-5220 to deal with > it. > > As Vinod suggested: For now, you would need to manually kill the > `forkedPid` similar to what is being done here: > https://github.com/apache/mesos/blob/master/src/tests/slave_recovery_tests.cpp#L2484 > > Can you also add a comment adding a `TODO` linking to MESOS-5220 once you > do this change so that this is tracked?
Thanks Anand, I have posted a separate patch for the test `SlaveRecoveryTest.RecoverTerminatedHTTPExecutor`: https://reviews.apache.org/r/46609/. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45670/#review128535 ----------------------------------------------------------- On April 13, 2016, 5:15 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45670/ > ----------------------------------------------------------- > > (Updated April 13, 2016, 5:15 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-3558 > https://issues.apache.org/jira/browse/MESOS-3558 > > > Repository: mesos > > > Description > ------- > > Updated tests for HTTP command executor. > > > Diffs > ----- > > src/tests/command_executor_tests.cpp > 843233adaa68ab1f5cedb7b075439bb8b77469a8 > > Diff: https://reviews.apache.org/r/45670/diff/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
