----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23443/#review47701 -----------------------------------------------------------
src/launcher/executor.cpp <https://reviews.apache.org/r/23443/#comment83855> This is a hack which assumes autotools layout. At the very least we should have a comment here that explains what is being done. ;-) That being said, I remember at one point you had an environment variable that I told you to remove. That would have been much more generic then this, my apologies. My concern with the environment variable was that it wasn't very explicit and therefore discoverable. That is, if someone else launched the mesos-executor they wouldn't know they couldn't pass a task with a health check unless you set the environment variable (and they'd have to look in the code to figure that out)! So it seems like we should always use dirname(argv[0]), which should work for the installed case. But also provide the environment variable to override dirname[argv[0]). Even better than an environment variable would be a flag, but I think that might be more complicated to take on. Given that this is an extension of 'launcher_dir' from the slave, how about naming that environment variable (or flag) MESOS_LAUNCHER_DIR as well? Just a little different than what you originally had, sorry for my lack of foresight in the earlier comments, thanks Tim. src/tests/health_check_tests.cpp <https://reviews.apache.org/r/23443/#comment83854> I don't understand how changing to ASSERT from EXPECT would change the flakiness. - Benjamin Hindman On July 13, 2014, 10:22 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23443/ > ----------------------------------------------------------- > > (Updated July 13, 2014, 10:22 p.m.) > > > Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen. > > > Bugs: MESOS-1533 > https://issues.apache.org/jira/browse/MESOS-1533 > > > Repository: mesos-git > > > Description > ------- > > Currently the health check tests are disabled as it was flaky. Part of the > reason is that all assertions was EXPECTs instead of ASSERTs so it will > continue to execute. > Another issue was it's currently discovering the path to launch health check > executable from the command executor main argv[0] path. > However, the correct path is to launch the generated script that does library > path resolution, which is the parent of .libs. > > Also changed the command executor to also include the health state when it > kills the task due to unhealthy checks. > > > Diffs > ----- > > src/launcher/executor.cpp 9c80848 > src/tests/health_check_tests.cpp 44711fd > > Diff: https://reviews.apache.org/r/23443/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
