----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51560/#review150374 -----------------------------------------------------------
src/health-check/health_checker.cpp (lines 185 - 189) <https://reviews.apache.org/r/51560/#comment218350> 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. src/tests/health_check_tests.cpp (line 212) <https://reviews.apache.org/r/51560/#comment218351> s/compatibility reasons/backwards compatibility/ so it's more clear. src/tests/health_check_tests.cpp (line 226) <https://reviews.apache.org/r/51560/#comment218352> s/compatibility reasons/backwards compatibility/ so it's more clear. - Jiang Yan Xu 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 > >
