mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 57535: Applied RegisterAgent ACL to the master.
Date Fri, 24 Mar 2017 21:58:59 GMT

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



Nice tests! Overall approach looks good to me. A few comments below.

Unrelated to your changes I noticed a few issues:

There are some inconsistencies between the framework and agent paths. For example, we don't
log when we receive an agent's (re-)registration message but we log the framework's subscription,
not sure why we did that. Also, since we don't track a framework's pending subscription, if
the authorization futures are re-ordered we could process subscre 2 before subscre 1, but
this is unrelated to your change here.

The "queueing up" logic (example [here](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L5344-L5345))
also allows re-ordering.


src/master/master.hpp
Line 1640 (original), 1665-1668 (patched)
<https://reviews.apache.org/r/57535/#comment242827>

    It looks like the comment about not answering questions about these transitioning agents
was removed, can we restore it?



src/master/master.cpp
Lines 3642-3656 (patched)
<https://reviews.apache.org/r/57535/#comment242828>

    Any reason there's no logging here? It would be nice to log consistently with the framework
authorization path:
    
    https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2201-L2203



src/master/master.cpp
Lines 3654 (patched)
<https://reviews.apache.org/r/57535/#comment242829>

    Maybe a little comment about why we don't have a request object here?


- Benjamin Mahler


On March 22, 2017, 12:48 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin Mahler, Greg
Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
>     https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/tests/master_authorization_tests.cpp 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/5/
> 
> 
> Testing
> -------
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will add them
when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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