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 57646: Made COMMAND health checks resilient to agent failovers.
Date Wed, 22 Mar 2017 18:21:22 GMT


> On March 22, 2017, 4:37 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 380-381 (patched)
> > <https://reviews.apache.org/r/57646/diff/2/?file=1670858#file1670858line380>
> >
> >     Please `<<` and escape task id.

Fixed in https://reviews.apache.org/r/57854/


> On March 22, 2017, 4:37 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 503-509 (patched)
> > <https://reviews.apache.org/r/57646/diff/2/?file=1670858#file1670858line504>
> >
> >     I'm not sure we should keep it. Wihtout knowing all the offline discussions
we had, this comment can be misleading, e.g., which future exactly do you mean or why would
we say `associate` if some failures should be mapped to discards.
> >     
> >     My understanding is that you use an extra promise here becase there are two
different events, which does not always conincide with their states, i.e., _some_ connection
failures should map to health result discards.
> >     
> >     I'd rather say what the promise you introduce represents, e.g., "Represents
the result of a health check".

Updated the comment.


> On March 22, 2017, 4:37 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 513 (patched)
> > <https://reviews.apache.org/r/57646/diff/2/?file=1670858#file1670858line514>
> >
> >     Do you really need to capture `this`?

We need it because we use `taskId`, we could cache it.... should we? If so, why is that prefered?


> On March 22, 2017, 4:37 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 514-515 (patched)
> > <https://reviews.apache.org/r/57646/diff/2/?file=1670858#file1670858line515>
> >
> >     Please carry space onto the next line.

Fixed in https://reviews.apache.org/r/57854/


> On March 22, 2017, 4:37 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 586-587 (patched)
> > <https://reviews.apache.org/r/57646/diff/2/?file=1670858#file1670858line587>
> >
> >     Please carry space onto the next line; escape task id

Fixed in https://reviews.apache.org/r/57854/


> On March 22, 2017, 4:37 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 681-684 (patched)
> > <https://reviews.apache.org/r/57646/diff/2/?file=1670858#file1670858line693>
> >
> >     This comment includes non-local information which tends to become stale. Instead,
how about we simply return `Failure(failure)` here and remove the comment?

If you do this, then the return will trigger the `onFailure` callback that discards the `Promise`.
=/


- Gastón


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


On March 22, 2017, 6:19 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57646/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 6:19 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
> -------
> 
> Made COMMAND health checks resilient to agent failovers.
> 
> 
> Diffs
> -----
> 
>   src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867 
>   src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23 
> 
> 
> Diff: https://reviews.apache.org/r/57646/diff/3/
> 
> 
> Testing
> -------
> 
> `make check` in Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


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