> On March 11, 2015, 4:02 a.m., Adam B wrote: > > src/hook/manager.cpp, line 137 > > <https://reviews.apache.org/r/31889/diff/5-6/?file=890977#file890977line137> > > > > What should you do if result.isNone()? Just ignore without warning (as > > you're already doing here)?
Yes, the hook decided to not add any new labels to the TaskInfo and that should be perfectly legit. That's why the Result type and not a Try type. > On March 11, 2015, 4:02 a.m., Adam B wrote: > > src/tests/hook_tests.cpp, line 199 > > <https://reviews.apache.org/r/31889/diff/6/?file=891026#file891026line199> > > > > Does this being a HookTest mean that it's guaranteed to use the > > "FOO=bar" test executorEnvironmentDecorator hook? I am not sure if I understand your question here. The test hook sets "FOO=bar" in the slaveExecutorEnvironmentDecorator and that's what we verify here. This test (along with other tests) is tightly coupled with the test hook module. > On March 11, 2015, 4:02 a.m., Adam B wrote: > > src/tests/hook_tests.cpp, line 234 > > <https://reviews.apache.org/r/31889/diff/6/?file=891026#file891026line234> > > > > Where do you actually validate the return value here? Is it just that > > you're expecting TASK_FINISHED instead of TASK_FAILED? Yes. I verified the result with "test $FOO = 'bbar'" and it results in TASK_FAILED instead of TASK_FINISHED. > On March 11, 2015, 4:02 a.m., Adam B wrote: > > src/tests/hook_tests.cpp, line 253 > > <https://reviews.apache.org/r/31889/diff/6/?file=891026#file891026line253> > > > > Could use the CREATE_EXECUTOR_INFO() + LaunchTasks() model instead of > > building up a TaskInfo yourself. Good point. Will update the diffs. - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31889/#review76049 ----------------------------------------------------------- On March 10, 2015, 10:15 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31889/ > ----------------------------------------------------------- > > (Updated March 10, 2015, 10:15 p.m.) > > > Review request for mesos, Adam B, Ben Mahler, Niklas Nielsen, and Till > Toenshoff. > > > Bugs: MESOS-2463 > https://issues.apache.org/jira/browse/MESOS-2463 > > > Repository: mesos > > > Description > ------- > > The ExecutorInfo is immutable, and so the environment decoration is deferred > until containerizer launch. The new slaveExecutorHook simply notifies the hook > module of a new executor being launched. The environment decorator hook for > containerizer launch update it's environment variables as gathered from the > hook > modules. > > > Diffs > ----- > > include/mesos/hook.hpp d83ace576a2c78eb7b1e910d89d912f6df5c46ef > src/examples/test_hook_module.cpp 8faf6850aafcebda7e9f0d1b735d61f7effa842d > src/hook/manager.hpp d3729eaca1bf2893cbc94cf62835dafb2d9a22a1 > src/hook/manager.cpp 3fd9d5e7f81e3d0ca2aca4091beba4ae555ae7e7 > src/slave/containerizer/containerizer.cpp > 33f8738fa229b42ebacfd75b762c977b33191b65 > src/slave/slave.cpp 364d911b086dfe1f15f76aa3888f99146aa8d876 > src/tests/hook_tests.cpp f4b4f519456dc00a8894c7ce154b28a7ab9ce493 > > Diff: https://reviews.apache.org/r/31889/diff/ > > > Testing > ------- > > make check. The master failover tests have not been performed yet. > > > Thanks, > > Kapil Arya > >
