mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jay Guo <guojiannan1...@gmail.com>
Subject Re: Review Request 58095: Refactored `Master::Http::roles` to use `jsonify`.
Date Mon, 08 May 2017 07:55:14 GMT


> On April 27, 2017, 10:58 p.m., Alexander Rojas wrote:
> > src/master/http.cpp
> > Line 3401 (original), 3403 (patched)
> > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3403>
> >
> >     your changes here make this function non safe thread. Notice that the original
would run in the context of the `MasterProcess` thread, while this will do so in the caller's
thread, causing a possible race condition.

Reverted to keep using `_roles()`.


> On April 27, 2017, 10:58 p.m., Alexander Rojas wrote:
> > src/master/http.cpp
> > Lines 3481-3482 (original), 3456-3457 (patched)
> > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3495>
> >
> >     Not yours, but I feel the formatting of this lambda is really off. The body
of the lambda starts a couple of columns earlier than declaration.

Other indents in this file are off too, e.g. `state()`, `frameworks()`


> On April 27, 2017, 10:58 p.m., Alexander Rojas wrote:
> > src/master/http.cpp
> > Line 3504 (original), 3505 (patched)
> > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3548>
> >
> >     Is ist really necesary to add empty fields?

Without this, `RoleTest.EndpointNoFrameworks` would be broken. I suppose we need empty fields
for backwards compatibility?


> On April 27, 2017, 10:58 p.m., Alexander Rojas wrote:
> > src/master/master.hpp
> > Lines 1399-1401 (original), 1399-1405 (patched)
> > <https://reviews.apache.org/r/58095/diff/4/?file=1693999#file1693999line1399>
> >
> >     How about rewording this to:
> >     
> >     ```c++
> >     // List of active roles, i.e. roles which are
> >     // explicitly white listed, plus roles with one or
> >     // more registered frameworks, plus all roles with a
> >     // non default weight or quota.
> >     ```
> >     
> >     This gives you a clear understanding of what the method returns without describing
how you do it (which is non interesting for API documentation). Also, the first paragraph
doesn't really says anything useful.

Left original method untouched.


- Jay


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


On May 8, 2017, 3:41 p.m., Jay Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> -----------------------------------------------------------
> 
> (Updated May 8, 2017, 3:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and Michael Park.
> 
> 
> Bugs: MESOS-4732 and MESOS-7260
>     https://issues.apache.org/jira/browse/MESOS-4732
>     https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored `Master::Http::roles` to use `jsonify`.
> 
> 
> Diffs
> -----
> 
>   src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e 
>   src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58095/diff/5/
> 
> 
> Testing
> -------
> 
> no functional changes
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>


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