> On Sept. 26, 2016, 5:22 p.m., Jiang Yan Xu wrote: > > src/health-check/health_checker.cpp, lines 185-189 > > <https://reviews.apache.org/r/51560/diff/14/?file=1509292#file1509292line185> > > > > How about the following? > > > > ``` > > if (check.has_command() && !check.has_http()) { > > check.set_type(HealthCheck::COMMAND); > > } else if (check.has_http() && !check.has_command()) { > > check.set_type(HealthCheck::HTTP); > > } else { > > ... > > } > > ``` > > > > It's obviously problematic to specify both but here we need to ensure > > that the behavior doesn't depend on the fact we look at `has_command()` > > first. > > > > The other thing is that it not ideal that we need to do this in two > > different places. In the codebase we have been more consistently doing the > > following: > > > > 1. Fill in missing fields for backwards compatibility and then > > 2. Keep the rest of the code free from such concerns. > > > > [One > > example](https://github.com/apache/mesos/blob/ec4c81a12559030791334359e7e1e2b6565cce01/src/master/master.cpp#L4066) > > > > Logically this block of code could be put directly below this example, > > i.e., just before task validation. > > > > ``` > > TaskInfo task_(task); > > if (task.has_executor() && !task.executor().has_framework_id()) { > > task_.mutable_executor() > > ->mutable_framework_id()->CopyFrom(framework->id()); > > } > > > > if (check.has_command()) { > > check.set_type(HealthCheck::COMMAND); > > } else if (check.has_http()) { > > check.set_type(HealthCheck::HTTP); > > } > > ``` > > > > Furthermore, it would be better if we extract these lines into a method. > > > > ``` > > TaskInfo adapt(const TaskInfo& task); > > ```` > > > > which takes care of all (past and future) such adjustments. I am not > > sure if devolve is the right place and we can put a TODO here and spend > > more time thinking about it outside this RR. > > Jiang Yan Xu wrote: > For the TODO I meant the "refactor into a method" part specifically.
Nice suggestions! Many thanks! - haosdent ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51560/#review150374 ----------------------------------------------------------- On Sept. 27, 2016, 3:38 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51560/ > ----------------------------------------------------------- > > (Updated Sept. 27, 2016, 3:38 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joseph Wu, Silas Snider, and > Jiang Yan Xu. > > > Bugs: MESOS-6110 > https://issues.apache.org/jira/browse/MESOS-6110 > > > Repository: mesos > > > Description > ------- > > Absence of `type` in command health check and HTTP health check are > supported for backwards compatibility and will be deprecated in > Mesos 2.0. > > > Diffs > ----- > > src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 > > Diff: https://reviews.apache.org/r/51560/diff/ > > > Testing > ------- > > Test case is https://reviews.apache.org/r/52301/ > > ``` > W0927 22:24:06.558188 117159 master.cpp:4079] The type of health check is not > set; use of 'HealthCheck' without specifying 'type' will be deprecated in > Mesos 2.0 > ... > [ OK ] HealthCheckTest.HealthyTaskViaHTTPWithoutType (946 ms) > [----------] 1 test from HealthCheckTest (972 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (1005 ms total) > [ PASSED ] 1 test. > ``` > > > Thanks, > > haosdent huang > >
