You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/07/15 22:30:16 UTC
Review Request 23520: Send healthy status when task killed after health check
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
-------
Currently after the health check receives configured amount of task fails it will initiate a kill task.
However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
This patch sets the health check field when the task is killed because of failing health checks.
Diffs
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23520/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47920
-----------------------------------------------------------
I enabled it locally and tested it myself :) Let me address these and update a new patch
- Timothy Chen
On July 15, 2014, 8:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 8:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47944
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On July 16, 2014, 12:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 12:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47942
-----------------------------------------------------------
Patch looks great!
Reviews applied: [23520]
All tests passed.
- Mesos ReviewBot
On July 16, 2014, 7:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47951
-----------------------------------------------------------
Ship it!
Ship It!
- Bernd Mathiske
On July 16, 2014, 12:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 12:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47943
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On July 16, 2014, 7:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/
-----------------------------------------------------------
(Updated July 16, 2014, 7:54 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
-------
Currently after the health check receives configured amount of task fails it will initiate a kill task.
However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
This patch sets the health check field when the task is killed because of failing health checks.
Diffs (updated)
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23520/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47910
-----------------------------------------------------------
Looks good - I added a couple of suggestions/questions and wonder how you tested it :-)
src/launcher/executor.cpp
<https://reviews.apache.org/r/23520/#comment84136>
The health check program may initiateTaskKill without the task status being unhealthy. How about setting this variable to 'healthy' unconditionally?
src/launcher/executor.cpp
<https://reviews.apache.org/r/23520/#comment84137>
unhealthy could be an option type, so you can encode both "killed healthy" and "killed unhealthy"
src/launcher/executor.cpp
<https://reviews.apache.org/r/23520/#comment84139>
Maybe make this an option type?
src/tests/health_check_tests.cpp
<https://reviews.apache.org/r/23520/#comment84135>
How did you test this if the test was disabled?
- Niklas Nielsen
On July 15, 2014, 1:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 1:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Timothy Chen <tn...@apache.org>.
> On July 16, 2014, 6:22 p.m., Bernd Mathiske wrote:
> > src/launcher/executor.cpp, line 320
> > <https://reviews.apache.org/r/23520/diff/1/?file=632791#file632791line320>
> >
> > Shouldn't unhealthy be set to true if the incoming parameter 'healthy' is false?
Hmm, it is a bit confusing and I wasn't able to come up with good names. This unhealthy flag is really meant that the health check process has marked this task to be killed after number of failed checks. Let me try to rename .
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47916
-----------------------------------------------------------
On July 15, 2014, 8:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 8:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Timothy Chen <tn...@apache.org>.
> On July 16, 2014, 6:22 p.m., Bernd Mathiske wrote:
> > src/launcher/executor.cpp, line 81
> > <https://reviews.apache.org/r/23520/diff/1/?file=632791#file632791line81>
> >
> > how about "healthy(true)" instead? Is the (double) negative necessary?
> >
I've renamed the flag now, let me know what you think.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47916
-----------------------------------------------------------
On July 15, 2014, 8:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 8:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/#review47916
-----------------------------------------------------------
src/launcher/executor.cpp
<https://reviews.apache.org/r/23520/#comment84149>
how about "healthy(true)" instead? Is the (double) negative necessary?
src/launcher/executor.cpp
<https://reviews.apache.org/r/23520/#comment84150>
Shouldn't unhealthy be set to true if the incoming parameter 'healthy' is false?
src/launcher/executor.cpp
<https://reviews.apache.org/r/23520/#comment84153>
Confusing. 'unhealthy' as used above does not encode task health. Neither does 'killed'.
- Bernd Mathiske
On July 15, 2014, 1:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23520/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 1:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently after the health check receives configured amount of task fails it will initiate a kill task.
> However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
> This patch sets the health check field when the task is killed because of failing health checks.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23520: Send healthy status when task killed after health
check
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23520/
-----------------------------------------------------------
(Updated July 15, 2014, 8:30 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
-------
Currently after the health check receives configured amount of task fails it will initiate a kill task.
However currently the task kill status update doesn't set the healthy field so it's unclear was it killed because health check or other reasons.
This patch sets the health check field when the task is killed because of failing health checks.
Diffs
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23520/diff/
Testing (updated)
-------
make check
Thanks,
Timothy Chen