mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gastón Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 55901: Added support for command health checks to the default executor.
Date Fri, 10 Feb 2017 18:41:26 GMT


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 653-654
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line653>
> >
> >     Why not `defer`?

Added `defer` where it corresponds.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 635
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line635>
> >
> >     Could you please specify what this function returns? Status or exit code?

This depends on what containerizer the agent is using. In the case of the Mesos containerizer,
it currently returns a status code.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 667
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line667>
> >
> >     What do you get from the agent call: status or exit code directly? It's probably
not that important here, but it is for checks.

This depends on what containerizer the agent is using. In the case of the Mesos containerizer,
it currently returns a status code.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 565-567
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line565>
> >
> >     This should probably be indented, no?

Fixed.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 555
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line555>
> >
> >     s/until/until after/ ?

Fixed.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 615
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line615>
> >
> >     s/this future is completed//we finish processing current timeout./
> >     
> >     It's not clear what "this future" means.

Fixed.


- Gastón


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164919
-----------------------------------------------------------


On Feb. 10, 2017, 6:40 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, and Vinod
Kone.
> 
> 
> Bugs: MESOS-6280
>     https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -----
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> -------
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on
Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message