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



Hey Till,

I have added some comments. Most are minor style sthings which you should feel 
free to ignore :-).

I have two high-level comments (we already talked about the first one but 
thought of putting it in here anyways):
1. Backwards compatibility wrt to the Role proto for /roles endpoint and the 
likes. 
2. I noticed that we have `master->roles.getRole()`, etc., scattered all over 
the code and wondering if we should put some of these methods in the Master 
class itself for better readability?


include/mesos/mesos.proto
Line 3407 (original), 3407-3409 (patched)
<https://reviews.apache.org/r/65939/#comment281329>

    I wonder how this would affect backwards compatibility. Do we need to keep 
`resources` as is and add `allocated` and `total_allocated` in addition? If we 
do this, we'd probably need to figure out a way to handle old vs new frameworks.



include/mesos/v1/master/master.proto
Lines 458-459 (original), 458-459 (patched)
<https://reviews.apache.org/r/65939/#comment281328>

    Please update the corresponding V0 master.proto as well.



src/master/master.hpp
Lines 139-140 (patched)
<https://reviews.apache.org/r/65939/#comment281362>

    Now that we are keeping track of `allocatedResources` and 
`totalAllocatedResources`, we can replace this hashmap with a simple hashset. 
We don't seem to be using `Framework*` elsewhere.



src/master/master.hpp
Lines 173-174 (patched)
<https://reviews.apache.org/r/65939/#comment281334>

    Can we change the third sentence ("An implicit ... active role.") to 
something like:
        An implicit role is not active - frameworks are connected to its child 
roles instead.



src/master/master.hpp
Lines 178 (patched)
<https://reviews.apache.org/r/65939/#comment281337>

    I am not sure if I understand what `returned in ["a", "a/b"]` means.



src/master/master.hpp
Lines 181 (patched)
<https://reviews.apache.org/r/65939/#comment281341>

    Would this search the framework in child roles as well? Can we expand to 
comment to explicitly mention it? Something like:
    
    ` Checks if the given framework is being tracked under the given role or 
its child roles`



src/master/master.cpp
Lines 495-501 (patched)
<https://reviews.apache.org/r/65939/#comment281345>

    Just a minor comment. Would it improve readability if we were to switch the 
if condition with the CHECK condition? As in:
    ```
    if (!roleMap.contains(path)) {
      CHECK(!parent->children.contains(path));
      VLOG(1) ...
      ...
    }
    ```



src/master/master.cpp
Lines 519 (patched)
<https://reviews.apache.org/r/65939/#comment281347>

    s/stop going up the tree/not removing it
    
    Here and below. That way it applies to the top-level roles as well.



src/master/master.cpp
Lines 520 (patched)
<https://reviews.apache.org/r/65939/#comment281346>

    s/break/return/ ?
    
    Here and below.



src/master/master.cpp
Lines 529 (patched)
<https://reviews.apache.org/r/65939/#comment281350>

    What if we bring this line after VLOG line. Further, we need a 
`CHECK_NOTNULL(parent);` here?



src/master/master.cpp
Lines 531 (patched)
<https://reviews.apache.org/r/65939/#comment281349>

    s/childless/leaf ?



src/master/master.cpp
Lines 572 (patched)
<https://reviews.apache.org/r/65939/#comment281360>

    Wondering if we should use `getRole(role)->frameworks.contains(id)` for 
uniformity.



src/master/master.cpp
Lines 587 (patched)
<https://reviews.apache.org/r/65939/#comment281361>

    Do we need a CHECK here, or is that too defensive?



src/master/master.cpp
Lines 619 (patched)
<https://reviews.apache.org/r/65939/#comment281363>

    This VLOG seems to stand out. Do we need similar log messages in other 
functions (`addResources`, etc.)?


- Kapil Arya


On April 5, 2018, 12:15 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> -----------------------------------------------------------
> 
> (Updated April 5, 2018, 12:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
> Michael Park, and Meng Zhu.
> 
> 
> Bugs: MESOS-8069
>     https://issues.apache.org/jira/browse/MESOS-8069
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When listing role "a", resources allocated to role "a/b" are added to
> those allocated to role "a". These changes affect both, the "/roles"
> endpoint as well as the V1 HTTP Call "GET_ROLES".
> Adds dynamic hierarchical role tracking to the master.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c 
>   src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c 
>   src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
>   src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1 
> 
> 
> Diff: https://reviews.apache.org/r/65939/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Reply via email to