mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Akash Gupta <akash-gu...@hotmail.com>
Subject Re: Review Request 65395: Refactored health checks to cleanly separate each different check.
Date Thu, 08 Feb 2018 17:54:17 GMT


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.hpp
> > Lines 143-148 (original)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950439#file1950439line174>
> >
> >     Leaving a note to make sure this comment was kept around.

thanks for catching this.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 253 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950440#file1950440line284>
> >
> >     s/E,F/E, F/g (See, I read the whole comment block!)
> >     
> >     Hm, I am left wondering what the `/-` means for the `Nested` row. I assume that
it means "not a thing on Windows", but we should probably exlicitly state it.

yeah it means "not implemented", but I'll add a note.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 258-271 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950440#file1950440line289>
> >
> >     I'm not generally a fan of capture-automatically semantics. I'm left wondering
what exactly we're capturing by copy here, if anything (`this` maybe?).

You need `this` and the check (`check::Command& cmd` in this case). For the capture semantics,
I'm interpretting this statement from the mesos style guide, "Prefer default capture by value,
explicit capture by value, then capture by reference. To avoid dangling-pointer bugs, never
use default capture by reference," as to acutally prefer capture-automatically semantics for
value types.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 415 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950440#file1950440line455>
> >
> >     Is this even reachable?

Yeah the above return is `#ifdef __linux__`, so this part is hit by non Linux platforms. Logically,
it won't probably get hit since Windows doesn't have namespaces, but the compiler doesn't
know that.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 435 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950440#file1950440line476>
> >
> >     Ditto above (sorry). Though that type is awful enough, perhaps we want to make
an exception.

Yeah, it's pretty ugly, but I guess I'll still add the type back to the left.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 807 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950440#file1950440line848>
> >
> >     IIRC this is a struct, should this be a const-ref?

We discussed this and it's best to keep passing these by value to avoid dangling refs.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/health_checker.cpp
> > Lines 187-214 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950442#file1950442line237>
> >
> >     Where else did I read this code...

Yup, I'm gonna move it inside the CheckerProcess to avoid code duplication.


> On Feb. 2, 2018, 9:27 p.m., Andrew Schwartzmeyer wrote:
> > src/docker/executor.cpp
> > Lines 658-664 (patched)
> > <https://reviews.apache.org/r/65395/diff/2/?file=1950443#file1950443line658>
> >
> >     Nit: Style-wise, can we brace-construct this whole struct? (Should we? Could
it become const-qualified if we did?)

yeah it can be const.


- Akash


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


On Feb. 8, 2018, 5:49 p.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2018, 5:49 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
>     https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored health check code to separate the logic for each check
> type and runtime (Plain/Docker/Nested). Now the matrix of different
> possiblilites is cleanly enumerated, making it easier to add
> future health checks, such as the ones for Windows.
> 
> 
> Diffs
> -----
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/65395/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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