----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59375/#review175482 -----------------------------------------------------------
Thanks for the patch, Andrei! First of all, let's address some process issues. We reference the JIRA ticket in the review, please do that. We also mention a review in the ticket, so that those who watch the ticket are given the opportunity to have a look at your patch. I'm sure you did some testing around your patch, could you please mention it in the appropriate section? I see that you modify only unversioned API. Is it intentional? There are two more tests using health status updates: `CommandCheckAndHealthCheckNoShadowing`. Could you please update those as well? include/mesos/mesos.proto Lines 1870 (patched) <https://reviews.apache.org/r/59375/#comment248920> Any reason you do not update v1 API as well? src/docker/executor.cpp Lines 486-487 (patched) <https://reviews.apache.org/r/59375/#comment248923> See my comment above. src/launcher/default_executor.cpp Lines 795-797 (original), 795-799 (patched) <https://reviews.apache.org/r/59375/#comment248921> Why do you think the reason here should be updated task health? This update is sent for the finished task, so the reason is more "task finished". We do include health information here, but I'm not sure we should modify the reason. src/launcher/executor.cpp Lines 917-920 (original), 917-921 (patched) <https://reviews.apache.org/r/59375/#comment248922> See my comment above. src/tests/health_check_tests.cpp Lines 479-481 (patched) <https://reviews.apache.org/r/59375/#comment248924> Why do you use asserts here and everywhere? - Alexander Rukletsov On May 18, 2017, 8 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59375/ > ----------------------------------------------------------- > > (Updated May 18, 2017, 8 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > ------- > > This gives clear indication about the reason of task status updates > caused by task health update. > > > Diffs > ----- > > include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154 > src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751 > src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2 > src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 > src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad > > > Diff: https://reviews.apache.org/r/59375/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Budnik > >
