You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Connor Doyle <co...@mesosphere.io> on 2015/01/06 00:17:48 UTC
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/
-----------------------------------------------------------
(Updated Jan. 5, 2015, 11:17 p.m.)
Review request for mesos, Adam B and Timothy Chen.
Changes
-------
Added the mesos group to the review.
--bmahler
Bugs: MESOS-2206
https://issues.apache.org/jira/browse/MESOS-2206
Repository: mesos-git
Description
-------
Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
- Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
- Pass known health to createTaskStatus during implicit and explicit task reconciliation.
Diffs
-----
src/common/protobuf_utils.hpp e42aaa5
src/common/protobuf_utils.cpp 8ab5cdd
src/master/master.cpp d6651e2
Diff: https://reviews.apache.org/r/29556/diff/
Testing
-------
- make
- make check
Manual testing using a development branch of the Marathon[1] framework:
- Launched two tasks with health checks.
- Waited for both tasks to enter the running state.
- Waited for both tasks to report healthy.
- Shut down the scheduler.
- Re-started (re-registered) the scheduler.
- Observed indeterminate health state for both tasks as reported by the framework.
- Waited for the framework to initiate task reconciliation.
- Observed a TaskStatus with the healthy field set to `true` for both tasks.
[1] http://github.com/mesosphere/marathon
Thanks,
Connor Doyle
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66753
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29556]
All tests passed.
- Mesos ReviewBot
On Jan. 5, 2015, 11:17 p.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 11:17 p.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66799
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On Jan. 6, 2015, 7:28 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 7:28 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66807
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29556]
All tests passed.
- Mesos ReviewBot
On Jan. 6, 2015, 7:28 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 7:28 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Connor Doyle <co...@mesosphere.io>.
> On Jan. 7, 2015, 10:37 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.cpp, line 119
> > <https://reviews.apache.org/r/29556/diff/3/?file=807607#file807607line119>
> >
> > Whoops, please put the brace on the next line!
> >
> > Also, could we call this `getTaskHealth` or at least `latestTaskHealth`?
Addressed these review comments in RR #29684 since this has already been committed.
- Connor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review67110
-----------------------------------------------------------
On Jan. 6, 2015, 7:28 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 7:28 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review67110
-----------------------------------------------------------
src/common/protobuf_utils.cpp
<https://reviews.apache.org/r/29556/#comment110933>
Whoops, please put the brace on the next line!
Also, could we call this `getTaskHealth` or at least `latestTaskHealth`?
src/common/protobuf_utils.cpp
<https://reviews.apache.org/r/29556/#comment110935>
Mind removing this variable?
src/tests/health_check_tests.cpp
<https://reviews.apache.org/r/29556/#comment110936>
Mind adding a newline here for readability?
- Ben Mahler
On Jan. 6, 2015, 7:28 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 7:28 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Connor Doyle <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/
-----------------------------------------------------------
(Updated Jan. 6, 2015, 7:28 a.m.)
Review request for mesos, Adam B and Timothy Chen.
Bugs: MESOS-2206
https://issues.apache.org/jira/browse/MESOS-2206
Repository: mesos-git
Description
-------
Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
- Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
- Pass known health to createTaskStatus during implicit and explicit task reconciliation.
Diffs (updated)
-----
src/common/protobuf_utils.hpp e42aaa5
src/common/protobuf_utils.cpp 8ab5cdd
src/master/master.cpp d6651e2
src/tests/health_check_tests.cpp a707398
Diff: https://reviews.apache.org/r/29556/diff/
Testing
-------
- make
- make check
Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
Manual testing using a development branch of the Marathon[1] framework:
- Launched two tasks with health checks.
- Waited for both tasks to enter the running state.
- Waited for both tasks to report healthy.
- Shut down the scheduler.
- Re-started (re-registered) the scheduler.
- Observed indeterminate health state for both tasks as reported by the framework.
- Waited for the framework to initiate task reconciliation.
- Observed a TaskStatus with the healthy field set to `true` for both tasks.
[1] http://github.com/mesosphere/marathon
Thanks,
Connor Doyle
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Connor Doyle <co...@mesosphere.io>.
> On Jan. 6, 2015, 6:47 a.m., Timothy Chen wrote:
> > src/common/protobuf_utils.hpp, line 58
> > <https://reviews.apache.org/r/29556/diff/2/?file=807500#file807500line58>
> >
> > This seems like you can just declare the function in cpp without adding it in the headers, but this is fine with me too.
>
> Adam B wrote:
> True that, but it's written generically enough that it could make sense in protobuf_utils, if any other file (registrar?) chose to use it. But I really doubt it'll get used outside of master, so maybe it is better to keep it hidden until it needs to be exposed. I could go either way.
Left it as-is for now, let me know if either of you want me to remove the declaration from the header file.
- Connor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66795
-----------------------------------------------------------
On Jan. 6, 2015, 6:13 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 6:13 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 5, 2015, 10:47 p.m., Timothy Chen wrote:
> > src/common/protobuf_utils.hpp, line 58
> > <https://reviews.apache.org/r/29556/diff/2/?file=807500#file807500line58>
> >
> > This seems like you can just declare the function in cpp without adding it in the headers, but this is fine with me too.
True that, but it's written generically enough that it could make sense in protobuf_utils, if any other file (registrar?) chose to use it. But I really doubt it'll get used outside of master, so maybe it is better to keep it hidden until it needs to be exposed. I could go either way.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66795
-----------------------------------------------------------
On Jan. 5, 2015, 10:13 p.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 10:13 p.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66795
-----------------------------------------------------------
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/29556/#comment110417>
This seems like you can just declare the function in cpp without adding it in the headers, but this is fine with me too.
src/tests/health_check_tests.cpp
<https://reviews.apache.org/r/29556/#comment110415>
Probably leave a comment why you choose to put TASK_KILLED here.
src/tests/health_check_tests.cpp
<https://reviews.apache.org/r/29556/#comment110416>
I think leaving a comment about the implicit vs explicit is a good idea, as it's not obvious what's going on
- Timothy Chen
On Jan. 6, 2015, 6:13 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 6:13 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66793
-----------------------------------------------------------
Ship it!
LGTM.
src/master/master.cpp
<https://reviews.apache.org/r/29556/#comment110412>
`const Option<ExecutorID>& executorId` please, (emphasis on the `&`), if the compiler allows it (I'm not sure about the magic of None()).
src/tests/health_check_tests.cpp
<https://reviews.apache.org/r/29556/#comment110413>
Shouldn't this guy also check has_healthy()?
- Adam B
On Jan. 5, 2015, 10:13 p.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 10:13 p.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Connor Doyle <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/
-----------------------------------------------------------
(Updated Jan. 6, 2015, 6:13 a.m.)
Review request for mesos, Adam B and Timothy Chen.
Bugs: MESOS-2206
https://issues.apache.org/jira/browse/MESOS-2206
Repository: mesos-git
Description
-------
Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
- Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
- Pass known health to createTaskStatus during implicit and explicit task reconciliation.
Diffs
-----
src/common/protobuf_utils.hpp e42aaa5
src/common/protobuf_utils.cpp 8ab5cdd
src/master/master.cpp d6651e2
src/tests/health_check_tests.cpp a707398
Diff: https://reviews.apache.org/r/29556/diff/
Testing (updated)
-------
- make
- make check
Modified one of the health check tests to ensure that the TaskStatus induced by implicit and explicit reconciliation have the health field set properly.
Manual testing using a development branch of the Marathon[1] framework:
- Launched two tasks with health checks.
- Waited for both tasks to enter the running state.
- Waited for both tasks to report healthy.
- Shut down the scheduler.
- Re-started (re-registered) the scheduler.
- Observed indeterminate health state for both tasks as reported by the framework.
- Waited for the framework to initiate task reconciliation.
- Observed a TaskStatus with the healthy field set to `true` for both tasks.
[1] http://github.com/mesosphere/marathon
Thanks,
Connor Doyle
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/#review66790
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29556]
All tests passed.
- Mesos ReviewBot
On Jan. 6, 2015, 5:29 a.m., Connor Doyle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29556/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 5:29 a.m.)
>
>
> Review request for mesos, Adam B and Timothy Chen.
>
>
> Bugs: MESOS-2206
> https://issues.apache.org/jira/browse/MESOS-2206
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
>
> - Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
> - Pass known health to createTaskStatus during implicit and explicit task reconciliation.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp e42aaa5
> src/common/protobuf_utils.cpp 8ab5cdd
> src/master/master.cpp d6651e2
> src/tests/health_check_tests.cpp a707398
>
> Diff: https://reviews.apache.org/r/29556/diff/
>
>
> Testing
> -------
>
> - make
> - make check
>
> Manual testing using a development branch of the Marathon[1] framework:
>
> - Launched two tasks with health checks.
> - Waited for both tasks to enter the running state.
> - Waited for both tasks to report healthy.
> - Shut down the scheduler.
> - Re-started (re-registered) the scheduler.
> - Observed indeterminate health state for both tasks as reported by the framework.
> - Waited for the framework to initiate task reconciliation.
> - Observed a TaskStatus with the healthy field set to `true` for both tasks.
>
> [1] http://github.com/mesosphere/marathon
>
>
> Thanks,
>
> Connor Doyle
>
>
Re: Review Request 29556: Send health state in TaskStatus during
reconciliation if known
Posted by Connor Doyle <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29556/
-----------------------------------------------------------
(Updated Jan. 6, 2015, 5:29 a.m.)
Review request for mesos, Adam B and Timothy Chen.
Bugs: MESOS-2206
https://issues.apache.org/jira/browse/MESOS-2206
Repository: mesos-git
Description
-------
Task statuses sent to the framework scheduler from the master do not include a value for the `healthy` field. The result is that the scheduler is left with incomplete knowledge about the health state of running tasks indefinitely, until by chance the executor sends a new status update.
- Update the createTaskStatus auxilliary function in protobuf utils to take an optional health flag.
- Pass known health to createTaskStatus during implicit and explicit task reconciliation.
Diffs (updated)
-----
src/common/protobuf_utils.hpp e42aaa5
src/common/protobuf_utils.cpp 8ab5cdd
src/master/master.cpp d6651e2
src/tests/health_check_tests.cpp a707398
Diff: https://reviews.apache.org/r/29556/diff/
Testing
-------
- make
- make check
Manual testing using a development branch of the Marathon[1] framework:
- Launched two tasks with health checks.
- Waited for both tasks to enter the running state.
- Waited for both tasks to report healthy.
- Shut down the scheduler.
- Re-started (re-registered) the scheduler.
- Observed indeterminate health state for both tasks as reported by the framework.
- Waited for the framework to initiate task reconciliation.
- Observed a TaskStatus with the healthy field set to `true` for both tasks.
[1] http://github.com/mesosphere/marathon
Thanks,
Connor Doyle