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 57110: Updated the master to handle frameworks that changes its roles.
Date Fri, 03 Mar 2017 04:26:14 GMT

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




src/master/master.hpp
Lines 2260-2263 (patched)
<https://reviews.apache.org/r/57110/#comment239753>

    This case is just one possibility, it's also possible that the framework removes the role
with tasks running still, or an agent that still needed to register but wasn't partitioned
reports back. So maybe something more general?
    
    ```
          // It's possible that we're not tracking the task's role for
          // this framework if the role is absent from the framework's
          // set of roles. In this case, we track the role's allocation
          // for this framework.
    ```



src/master/master.hpp
Lines 2263 (patched)
<https://reviews.apache.org/r/57110/#comment239752>

    This last sentence seems a bit redundant?



src/master/master.hpp
Lines 2320-2324 (original), 2355-2375 (patched)
<https://reviews.apache.org/r/57110/#comment239755>

    Can we just call `recoverResources(task)` here?



src/master/master.hpp
Lines 2448-2451 (patched)
<https://reviews.apache.org/r/57110/#comment239754>

    Ditto here.



src/master/master.hpp
Lines 2526 (patched)
<https://reviews.apache.org/r/57110/#comment239756>

    At this point, you have based it on `info` instead of `source`, right? Maybe that's a
little clearer since we just adjusted `info`?



src/master/master.cpp
Lines 2637-2638 (original), 2637-2638 (patched)
<https://reviews.apache.org/r/57110/#comment239757>

    Do you need the quotes here? This ends up coming out like this:
    
    ```
    Could not update FrameworkInfo of framework '<ID> (hostname) at 10.10.10.10:5050':
...
    ```
    
    Whereas I think your intention was to quote the ID. Seems like this is the responsibility
of the output operator for `Framework&`.



src/master/master.cpp
Lines 2951-2952 (original), 2941-2942 (patched)
<https://reviews.apache.org/r/57110/#comment239758>

    Oh I see it was copied from here.. we should fix the quoting here as well.



src/master/master.cpp
Lines 3301 (patched)
<https://reviews.apache.org/r/57110/#comment239742>

    s/std:://
    
    Also I think it fits on one line at that point, like the others below.



src/master/master.cpp
Lines 3335-3337 (patched)
<https://reviews.apache.org/r/57110/#comment239741>

    Should we use .at() for these three lines that intend const access into the map?



src/master/master.cpp
Lines 5992-5993 (patched)
<https://reviews.apache.org/r/57110/#comment239759>

    Per our offline discussion, looks like this belongs in one of the previous reviews that
addresses the addition of the FrameworkInfo into the message? Seems like we need to send it
always now that it includes the info.



src/master/master.cpp
Lines 6085-6091 (patched)
<https://reviews.apache.org/r/57110/#comment239761>

    This could be simplified to:
    
    ```
    const set<string> removedRoles = oldRoles - newRoles;
    ```
    
    If we were to add the `-` operator to the existing operators in stout/set.hpp.



src/master/master.cpp
Lines 6116-6118 (patched)
<https://reviews.apache.org/r/57110/#comment239763>

    And we'll expose this via metrics right? Maybe just say that we'll still account for the
allocation to the role for the framework?



src/master/master.cpp
Lines 6124-6130 (patched)
<https://reviews.apache.org/r/57110/#comment239762>

    Ditto here, could be:
    
    ```
      const set<string> addedRoles = newRoles - oldRoles;
    ```



src/master/master.cpp
Lines 6133-6136 (patched)
<https://reviews.apache.org/r/57110/#comment239764>

    How about a newline between the comment blocks for readability?
    
    ```
        // We only add the framework to the role if it doesn't already exist.
        //
        // NOTE: It's possible that the framework already exists in the role since
        // we don't remove the framework from the role until there are no resources
        // being used by the framework within that role.
    ```
    
    Ditto on the other notes.



src/master/master.cpp
Line 7453 (original), 7568 (patched)
<https://reviews.apache.org/r/57110/#comment239743>

    Ditto here, this can just be:
    
    ```
      foreach (const string& role, framework->roles) {
        // ...
      }
    ```



src/master/master.cpp
Lines 7453-7460 (original), 7568-7576 (patched)
<https://reviews.apache.org/r/57110/#comment239749>

    Ditto here, I guess this case needs to be called from the constructor? Wonder if we need
a `Framework::initialize`?



src/master/master.cpp
Line 7549 (original), 7656 (patched)
<https://reviews.apache.org/r/57110/#comment239738>

    Should this be composing the message?
    
    ```
    return Error("Failed to update framework: " + updateFramework_.error());
    ```



src/master/master.cpp
Lines 7892-7893 (original), 7992-7993 (patched)
<https://reviews.apache.org/r/57110/#comment239739>

    No need to call the helper since we already maintain the set within the Framework struct.
This can just be:
    
    ```
      foreach (const string& role, framework->roles) {
        removeFrameworkFromRole(framework, role);
      }
    ```



src/master/master.cpp
Lines 7892-7894 (original), 7992-7994 (patched)
<https://reviews.apache.org/r/57110/#comment239747>

    Currently, these functions are sometimes called from the Framework struct and sometimes
from the Master, so the responsibilities between the master and the framework seem a little
unclear, and potentially error prone.
    
    Is it possible to place all of the calls into the role management into the framework struct?
From our conversation offline, it sounds like we'll need to introduce a function like `Framework::complete`
so that we can avoid calling `removeFrameworkFromRole` from the Framework destructor (which
doesn't work since we keep completed frameworks).


- Benjamin Mahler


On Feb. 27, 2017, 10:19 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2017, 10:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
>     https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
> 
> 
> Diff: https://reviews.apache.org/r/57110/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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