You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/02/02 09:56:28 UTC
Review Request 56208: Updated checks library with general check
support.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 1, 2017, 8:23 p.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 424 (original), 440 (patched)
> > <https://reviews.apache.org/r/56208/diff/5-7/?file=1628427#file1628427line452>
> >
> > why are we sending WEXITSTATUS and not exit code?
>
> Alexander Rukletsov wrote:
> Because what we get from `Subprocess` is actually `pid_t`. It is unclear what the scheduler will do with the pid.
>
> Vinod Kone wrote:
> AFAIK the status code returned by `waitpid()` has strictly more information (whether a process has exited, signalled or stopped). So I can imagine scheduler can make better decisions with extra information? It's a bit weird that we are not sending check status if a command exits because of signaling or stopping?
>
> Also, AFAIK we send status code in the executor terminated event as well? So it would be nice if we can be consistent unless there is a strong reason to differ.
Correcting my answer above. What we get from `Subprocess` is `status_value` (see man on `waitpid`), which embeds exit code and has extra information. I'm not sure whether a scheduler is interested in fine grained termination information for a check.
I would argue it may be more surprising for 3rdparty tools to see Posix's internal `status_value` instead of a plain exit code. Moreover, IIUC to extract exit code from `status_value`, a scheduler should know whether the message is coming from Posix or Windows agent and hence either apply extra interpretation or use the value as is.
However, I do confirm that executor termination event contains `status_value`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review167557
-----------------------------------------------------------
On March 7, 2017, 8:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 8:39 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
>
>
> Diff: https://reviews.apache.org/r/56208/diff/8/
>
>
> Testing
> -------
>
> https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 1, 2017, 8:23 p.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 424 (original), 440 (patched)
> > <https://reviews.apache.org/r/56208/diff/5-7/?file=1628427#file1628427line452>
> >
> > why are we sending WEXITSTATUS and not exit code?
>
> Alexander Rukletsov wrote:
> Because what we get from `Subprocess` is actually `pid_t`. It is unclear what the scheduler will do with the pid.
>
> Vinod Kone wrote:
> AFAIK the status code returned by `waitpid()` has strictly more information (whether a process has exited, signalled or stopped). So I can imagine scheduler can make better decisions with extra information? It's a bit weird that we are not sending check status if a command exits because of signaling or stopping?
>
> Also, AFAIK we send status code in the executor terminated event as well? So it would be nice if we can be consistent unless there is a strong reason to differ.
>
> Alexander Rukletsov wrote:
> Correcting my answer above. What we get from `Subprocess` is `status_value` (see man on `waitpid`), which embeds exit code and has extra information. I'm not sure whether a scheduler is interested in fine grained termination information for a check.
>
> I would argue it may be more surprising for 3rdparty tools to see Posix's internal `status_value` instead of a plain exit code. Moreover, IIUC to extract exit code from `status_value`, a scheduler should know whether the message is coming from Posix or Windows agent and hence either apply extra interpretation or use the value as is.
>
> However, I do confirm that executor termination event contains `status_value`.
See https://reviews.apache.org/r/57597
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review167557
-----------------------------------------------------------
On March 14, 2017, 2:05 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 2:05 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
>
>
> Diff: https://reviews.apache.org/r/56208/diff/9/
>
>
> Testing
> -------
>
> https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 1, 2017, 8:23 p.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 424 (original), 440 (patched)
> > <https://reviews.apache.org/r/56208/diff/5-7/?file=1628427#file1628427line452>
> >
> > why are we sending WEXITSTATUS and not exit code?
>
> Alexander Rukletsov wrote:
> Because what we get from `Subprocess` is actually `pid_t`. It is unclear what the scheduler will do with the pid.
AFAIK the status code returned by `waitpid()` has strictly more information (whether a process has exited, signalled or stopped). So I can imagine scheduler can make better decisions with extra information? It's a bit weird that we are not sending check status if a command exits because of signaling or stopping?
Also, AFAIK we send status code in the executor terminated event as well? So it would be nice if we can be consistent unless there is a strong reason to differ.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review167557
-----------------------------------------------------------
On March 7, 2017, 8:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 8:39 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
>
>
> Diff: https://reviews.apache.org/r/56208/diff/8/
>
>
> Testing
> -------
>
> https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 1, 2017, 8:23 p.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 424 (original), 440 (patched)
> > <https://reviews.apache.org/r/56208/diff/5-7/?file=1628427#file1628427line452>
> >
> > why are we sending WEXITSTATUS and not exit code?
Because what we get from `Subprocess` is actually `pid_t`. It is unclear what the scheduler will do with the pid.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review167557
-----------------------------------------------------------
On March 1, 2017, 12:50 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 12:50 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
>
>
> Diff: https://reviews.apache.org/r/56208/diff/7/
>
>
> Testing
> -------
>
> https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review167557
-----------------------------------------------------------
src/checks/checker.cpp
Line 297 (original), 309 (patched)
<https://reviews.apache.org/r/56208/#comment239434>
Just do EXIT or LOG(FATAL), doing a CHECK that obviously fails is weird.
```
LOG(FATAL) << "Received UNKNOWN check type";
```
src/checks/checker.cpp
Line 315 (original), 328 (patched)
<https://reviews.apache.org/r/56208/#comment239435>
we should probably have a "<<" operator overload for check type.
src/checks/checker.cpp
Lines 398 (patched)
<https://reviews.apache.org/r/56208/#comment239440>
can you add a comment on why you need to do a copy here?
src/checks/checker.cpp
Line 424 (original), 440 (patched)
<https://reviews.apache.org/r/56208/#comment239446>
why are we sending WEXITSTATUS and not exit code?
src/checks/checker.cpp
Lines 499 (patched)
<https://reviews.apache.org/r/56208/#comment239443>
ditto. comment here.
- Vinod Kone
On March 1, 2017, 12:50 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 12:50 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
>
>
> Diff: https://reviews.apache.org/r/56208/diff/7/
>
>
> Testing
> -------
>
> https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated March 15, 2017, 12:46 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Rebased. NNTR.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
Diff: https://reviews.apache.org/r/56208/diff/10/
Changes: https://reviews.apache.org/r/56208/diff/9-10/
Testing
-------
https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review168974
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On March 14, 2017, 2:05 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 2:05 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
>
>
> Diff: https://reviews.apache.org/r/56208/diff/9/
>
>
> Testing
> -------
>
> https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated March 14, 2017, 2:05 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
Diff: https://reviews.apache.org/r/56208/diff/9/
Changes: https://reviews.apache.org/r/56208/diff/8-9/
Testing
-------
https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated March 7, 2017, 8:39 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
Diff: https://reviews.apache.org/r/56208/diff/8/
Changes: https://reviews.apache.org/r/56208/diff/7-8/
Testing
-------
https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated March 1, 2017, 12:50 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Made sure command exit code is returned.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
Diff: https://reviews.apache.org/r/56208/diff/7/
Changes: https://reviews.apache.org/r/56208/diff/6-7/
Testing
-------
https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 3:50 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
Diff: https://reviews.apache.org/r/56208/diff/
Testing (updated)
-------
https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 3:49 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 11:12 a.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Feb. 14, 2017, 5:46 p.m., Gast�n Kleiman wrote:
> > src/checks/checker.cpp, line 219
> > <https://reviews.apache.org/r/56208/diff/5/?file=1628427#file1628427line219>
> >
> > I think that it'd be useful to log the `taskId` here, since an executor might have multiple checkers running at the same time.
>
> Alexander Rukletsov wrote:
> To do this we have to cache `taskId` in the class. Is it worth it?
As per offline discussion, I think that we could move this logging statement to `CheckerProcess::finalize()`.
I see the following benefits:
1. It makes the debugging output friendlier =).
2. `CheckerProcess::initialize()` already logs the check configuration, so logging something on termination would be symmetrical.
3. It makes `Checker` a plain and simple wrapper, except maybe for the extra validation in `Checker::create()`.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review165521
-----------------------------------------------------------
On Feb. 28, 2017, 11:12 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 11:12 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 14, 2017, 5:46 p.m., Gast�n Kleiman wrote:
> > src/checks/checker.cpp, line 219
> > <https://reviews.apache.org/r/56208/diff/5/?file=1628427#file1628427line219>
> >
> > I think that it'd be useful to log the `taskId` here, since an executor might have multiple checkers running at the same time.
>
> Alexander Rukletsov wrote:
> To do this we have to cache `taskId` in the class. Is it worth it?
>
> Gast�n Kleiman wrote:
> As per offline discussion, I think that we could move this logging statement to `CheckerProcess::finalize()`.
>
> I see the following benefits:
>
> 1. It makes the debugging output friendlier =).
> 2. `CheckerProcess::initialize()` already logs the check configuration, so logging something on termination would be symmetrical.
> 3. It makes `Checker` a plain and simple wrapper, except maybe for the extra validation in `Checker::create()`.
I'm convinced!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review165521
-----------------------------------------------------------
On Feb. 28, 2017, 11:12 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 11:12 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 14, 2017, 5:46 p.m., Gast�n Kleiman wrote:
> > Many of the comments here also apply to the current code in `health_checker.cpp`. One possibility would be to commit this patch without fixing them, and then creating a new patch with the fixes for both `health_checker.cpp` and `checker.cpp`.
My suggestion would be to fix everything here and refrain from fixing `health_checker.cpp` because of https://issues.apache.org/jira/browse/MESOS-7092.
> On Feb. 14, 2017, 5:46 p.m., Gast�n Kleiman wrote:
> > src/checks/checker.hpp, line 77
> > <https://reviews.apache.org/r/56208/diff/5/?file=1628426#file1628426line77>
> >
> > `Checker(const Checker&) = delete` seems to be more popular than making the constructor private.
> >
> > Is that idiom preferred?
Good idea. Fixed.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review165521
-----------------------------------------------------------
On Feb. 28, 2017, 11:12 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 11:12 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 14, 2017, 5:46 p.m., Gast�n Kleiman wrote:
> > src/checks/checker.cpp, line 219
> > <https://reviews.apache.org/r/56208/diff/5/?file=1628427#file1628427line219>
> >
> > I think that it'd be useful to log the `taskId` here, since an executor might have multiple checkers running at the same time.
To do this we have to cache `taskId` in the class. Is it worth it?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review165521
-----------------------------------------------------------
On Feb. 28, 2017, 11:12 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 11:12 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review165521
-----------------------------------------------------------
Many of the comments here also apply to the current code in `health_checker.cpp`. One possibility would be to commit this patch without fixing them, and then creating a new patch with the fixes for both `health_checker.cpp` and `checker.cpp`.
src/checks/checker.hpp (line 61)
<https://reviews.apache.org/r/56208/#comment237374>
Do we need to pass the `TaskID` to the callback? The `checker` is bound to a task, so the creator can do something like:
```
checker = Checker::create(
checkInfo,
defer(self(), &Self::checkStatusUpdated, taskId, lambda::_1),
...
);
```
src/checks/checker.hpp (line 77)
<https://reviews.apache.org/r/56208/#comment237361>
`Checker(const Checker&) = delete` seems to be more popular than making the constructor private.
Is that idiom preferred?
src/checks/checker.cpp (line 31)
<https://reviews.apache.org/r/56208/#comment237366>
Do we really need `startTime` (see two issues below)? If not, we can remove this.
src/checks/checker.cpp (line 65)
<https://reviews.apache.org/r/56208/#comment237367>
Ditto.
src/checks/checker.cpp (line 173)
<https://reviews.apache.org/r/56208/#comment237364>
Looks like a left-over from health checks. There it is used for the grace period, but here it is initialized, and then never used again.
src/checks/checker.cpp (line 219)
<https://reviews.apache.org/r/56208/#comment237379>
I think that it'd be useful to log the `taskId` here, since an executor might have multiple checkers running at the same time.
src/checks/checker.cpp (line 267)
<https://reviews.apache.org/r/56208/#comment237378>
Ditto `taskId`.
src/checks/checker.cpp (line 305)
<https://reviews.apache.org/r/56208/#comment237377>
Ditto `taskId`.
src/checks/checker.cpp (line 315)
<https://reviews.apache.org/r/56208/#comment237380>
Ditto `taskId`.
src/checks/checker.cpp (lines 339 - 344)
<https://reviews.apache.org/r/56208/#comment237384>
I know that what you put here is what we currently do for command health checks, but it would probably make sense to overwrite env variables set in `check.command().command().environment()`.
What we have here seems to be the equivalent of passing `environment = None()` to `subprocess()`.
src/checks/checker.cpp (line 351)
<https://reviews.apache.org/r/56208/#comment237381>
Ditto `taskId`.
src/checks/checker.cpp (line 365)
<https://reviews.apache.org/r/56208/#comment237382>
Ditto `taskId`.
src/checks/checker.cpp (line 383)
<https://reviews.apache.org/r/56208/#comment237387>
mark this `const`? or is it considered a POD?
src/checks/checker.cpp (line 394)
<https://reviews.apache.org/r/56208/#comment237388>
Ditto `taskId`.
src/checks/checker.cpp (line 420)
<https://reviews.apache.org/r/56208/#comment237389>
Ditto `taskId`.
src/checks/checker.cpp (line 428)
<https://reviews.apache.org/r/56208/#comment237390>
Ditto `taskId`.
src/checks/checker.cpp (line 450)
<https://reviews.apache.org/r/56208/#comment237391>
Ditto `taskId`.
src/checks/checker.cpp (line 481)
<https://reviews.apache.org/r/56208/#comment237392>
ditto marking it as `const`
src/checks/checker.cpp (line 497)
<https://reviews.apache.org/r/56208/#comment237393>
Ditto `taskId`.
src/checks/checker.cpp (line 570)
<https://reviews.apache.org/r/56208/#comment237395>
Ditto `taskId`.
- Gast�n Kleiman
On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 12:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review165034
-----------------------------------------------------------
src/checks/checker.cpp (line 255)
<https://reviews.apache.org/r/56208/#comment236876>
i would keep this inlined. making it a helper is more confusing than worth it IMO.
- Vinod Kone
On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 12:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 9, 2017, 12:56 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 9:06 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Simplified loop.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Gastón Kleiman <ga...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review164677
-----------------------------------------------------------
src/checks/checker.hpp (line 70)
<https://reviews.apache.org/r/56208/#comment236446>
s/taskID/taskId/
We don't use the name `taskID` anywhere else in Mesos.
src/checks/checker.hpp (line 95)
<https://reviews.apache.org/r/56208/#comment236447>
ditto capitalization of the name.
- Gast�n Kleiman
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 4:56 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 3:13 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.
Diffs (updated)
-----
src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
src/checks/health_checker.hpp b60666b86e260bc129ede52cd2eb18911c8fec29
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
Diff: https://reviews.apache.org/r/56208/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 7, 2017, 1:38 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp, line 273
> > <https://reviews.apache.org/r/56208/diff/1/?file=1622056#file1622056line273>
> >
> > is the equality operator defined for the above statement to be true?
I'm not sure I understand you question, but we've defined equality operator ourselves: https://github.com/apache/mesos/blob/64863a573caf72510db367e3df0b12a5a235647f/src/common/type_utils.cpp#L453-L462
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review164424
-----------------------------------------------------------
On Feb. 8, 2017, 3:13 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 3:13 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp b60666b86e260bc129ede52cd2eb18911c8fec29
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 7, 2017, 1:38 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp, line 273
> > <https://reviews.apache.org/r/56208/diff/1/?file=1622056#file1622056line273>
> >
> > is the equality operator defined for the above statement to be true?
>
> Alexander Rukletsov wrote:
> I'm not sure I understand you question, but we've defined equality operator ourselves: https://github.com/apache/mesos/blob/64863a573caf72510db367e3df0b12a5a235647f/src/common/type_utils.cpp#L453-L462
in that case just say "trigger an update if check info changes". "the value or presence" bit is redundant because that's what most/all our equality operators do.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review164424
-----------------------------------------------------------
On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 12:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56208: Updated checks library with general check
support.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56208/#review164424
-----------------------------------------------------------
src/checks/checker.hpp (line 60)
<https://reviews.apache.org/r/56208/#comment236129>
s/prior/prior to/
s/a check/the check/
src/checks/checker.hpp (line 71)
<https://reviews.apache.org/r/56208/#comment236128>
const & ?
src/checks/checker.hpp (line 88)
<https://reviews.apache.org/r/56208/#comment236130>
Any reason why this is defined here instead of inside the cpp file?
src/checks/checker.hpp (line 96)
<https://reviews.apache.org/r/56208/#comment236131>
const &?
src/checks/checker.hpp (line 105)
<https://reviews.apache.org/r/56208/#comment236144>
s/performSingleCheck/performCheck/ ?
similar to how you don't say `singleCommandCheck` or `singleHttpCheck` below
src/checks/checker.hpp (line 126)
<https://reviews.apache.org/r/56208/#comment236132>
const?
src/checks/checker.cpp (line 123)
<https://reviews.apache.org/r/56208/#comment236140>
extraneous back tick.
src/checks/checker.cpp (line 141)
<https://reviews.apache.org/r/56208/#comment236136>
put it on the previous line?
src/checks/checker.cpp (line 273)
<https://reviews.apache.org/r/56208/#comment236146>
is the equality operator defined for the above statement to be true?
src/checks/checker.cpp (line 274)
<https://reviews.apache.org/r/56208/#comment236145>
i.e.,
src/checks/checker.cpp (line 300)
<https://reviews.apache.org/r/56208/#comment236166>
s/external/s/ like you did below for http check?
- Vinod Kone
On Feb. 2, 2017, 9:56 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2017, 9:56 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
>
>
> Diffs
> -----
>
> src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277
> src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
>
> Diff: https://reviews.apache.org/r/56208/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>