> On Sept. 26, 2016, 10:22 a.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.
For the TODO I meant the "refactor into a method" part specifically. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51560/#review150374 ----------------------------------------------------------- On Sept. 23, 2016, 8:38 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51560/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2016, 8:38 a.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 > ------- > > Health checks must supply set type field from now on. Additionally, > `HealthCheck.HTTP` message has been renamed to > `HealthCheck.HttpCheckInfo` to avoid naming collisions. > > > Diffs > ----- > > src/health-check/health_checker.cpp > 736725c4ef954ece8580f383cfd31d289795903f > src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 > > Diff: https://reviews.apache.org/r/51560/diff/ > > > Testing > ------- > > Could verfied from > https://github.com/haosdent/mesos/blob/85abb6da058bf8daf4e794df7da8eb5f240b671c/docs/upgrades.md > > > Thanks, > > haosdent huang > >
