You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2017/05/18 20:00:42 UTC
Review Request 59375: Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED
as task reason. (WIP)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/
-----------------------------------------------------------
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
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/#review175610
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On May 19, 2017, 9:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 9:07 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6905
> https://issues.apache.org/jira/browse/MESOS-6905
>
>
> 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
> include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3
> src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751
> src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2
> src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4
> src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6
> src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad
>
>
> Diff: https://reviews.apache.org/r/59375/diff/3/
>
>
> Testing
> -------
>
> Added checks for task status updates caused by task health update.
>
> make check (fedora 25, mac os 10.12)
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/#review175598
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59375]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 19, 2017, 9:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 9:07 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6905
> https://issues.apache.org/jira/browse/MESOS-6905
>
>
> 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
> include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3
> src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751
> src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2
> src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4
> src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6
> src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad
>
>
> Diff: https://reviews.apache.org/r/59375/diff/3/
>
>
> Testing
> -------
>
> Added checks for task status updates caused by task health update.
>
> make check (fedora 25, mac os 10.12)
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/
-----------------------------------------------------------
(Updated May 19, 2017, 9:07 p.m.)
Review request for mesos and Alexander Rukletsov.
Bugs: MESOS-6905
https://issues.apache.org/jira/browse/MESOS-6905
Repository: mesos
Description
-------
This gives clear indication about the reason of task status updates
caused by task health update.
Diffs (updated)
-----
include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154
include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3
src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751
src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2
src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4
src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6
src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad
Diff: https://reviews.apache.org/r/59375/diff/3/
Changes: https://reviews.apache.org/r/59375/diff/2-3/
Testing
-------
Added checks for task status updates caused by task health update.
make check (fedora 25, mac os 10.12)
Thanks,
Andrei Budnik
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/#review175526
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59375]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 19, 2017, 2:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 2:54 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6905
> https://issues.apache.org/jira/browse/MESOS-6905
>
>
> 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
> include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3
> src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751
> src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2
> src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4
> src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6
> src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad
>
>
> Diff: https://reviews.apache.org/r/59375/diff/2/
>
>
> Testing
> -------
>
> Added checks for task status updates caused by task health update.
>
> make check (fedora 25, mac os 10.12)
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/
-----------------------------------------------------------
(Updated May 19, 2017, 2:54 p.m.)
Review request for mesos and Alexander Rukletsov.
Summary (updated)
-----------------
Added REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Bugs: MESOS-6905
https://issues.apache.org/jira/browse/MESOS-6905
Repository: mesos
Description
-------
This gives clear indication about the reason of task status updates
caused by task health update.
Diffs (updated)
-----
include/mesos/mesos.proto e138e2aedb1369acdde82facf9dfea9780411154
include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3
src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751
src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2
src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4
src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6
src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad
Diff: https://reviews.apache.org/r/59375/diff/2/
Changes: https://reviews.apache.org/r/59375/diff/1-2/
Testing
-------
Added checks for task status updates caused by task health update.
make check (fedora 25, mac os 10.12)
Thanks,
Andrei Budnik
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/
-----------------------------------------------------------
(Updated May 19, 2017, 2:30 p.m.)
Review request for mesos and Alexander Rukletsov.
Bugs: MESOS-6905
https://issues.apache.org/jira/browse/MESOS-6905
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 (updated)
-------
Added checks for task status updates caused by task health update.
make check (fedora 25, mac os 10.12)
Thanks,
Andrei Budnik
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/
-----------------------------------------------------------
(Updated May 19, 2017, 10:14 a.m.)
Review request for mesos and Alexander Rukletsov.
Bugs: MESOS-6905
https://issues.apache.org/jira/browse/MESOS-6905
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
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On May 19, 2017, 10:04 a.m., Alexander Rukletsov wrote:
> > src/launcher/default_executor.cpp
> > Lines 795-797 (original), 795-799 (patched)
> > <https://reviews.apache.org/r/59375/diff/1/?file=1724147#file1724147line795>
> >
> > 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.
After the discussion with AlexR, we've decided to consider specifying appropriate status update reason, meaning that the task was killed due to a failing health check. Something like TaskStatus::REASON_TASK_KILLED_AFTER_FAILING_HEALTH_CHECK.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/#review175482
-----------------------------------------------------------
On May 19, 2017, 9:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59375/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 9:07 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6905
> https://issues.apache.org/jira/browse/MESOS-6905
>
>
> 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
> include/mesos/v1/mesos.proto ab617bfd2bc46e44f7d71cefecf5eac3630d22b3
> src/docker/executor.cpp 82ae9bdc94565acb746724c2e6ab6432aa6bd751
> src/launcher/default_executor.cpp 5c31f948b5ba470e7c007c39e1c0c903451477e2
> src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4
> src/tests/check_tests.cpp 3607a690c963a84acfb2d3adc6c4a4f148b456d6
> src/tests/health_check_tests.cpp 6c1b9a0ead6edb34c20cf4973fffcff913c5d7ad
>
>
> Diff: https://reviews.apache.org/r/59375/diff/3/
>
>
> Testing
> -------
>
> Added checks for task status updates caused by task health update.
>
> make check (fedora 25, mac os 10.12)
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 59375: Added
REASON_TASK_HEALTH_CHECK_STATUS_UPDATED as task reason. (WIP)
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59375/#review175456
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59375]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>