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

Ship it!



src/master/master.cpp (lines 1719 - 1721)
<https://reviews.apache.org/r/37046/#comment148400>

    Hm.. for later, I suppose we'll want to have a metric helper for calls 
inside receive. We currently aren't counting the number of call messages, right?



src/master/master.cpp (line 1729)
<https://reviews.apache.org/r/37046/#comment148398>

    s/registration/subcribe/ ?



src/master/master.cpp 
<https://reviews.apache.org/r/37046/#comment148394>

    Hm.. is the idea that we can remove this because we trust that the 
scheduler driver will not set this for a register message?



src/master/master.cpp (lines 1749 - 1750)
<https://reviews.apache.org/r/37046/#comment148395>

    Hm.. if you're going to combine these mind at least wrapping so that the 
root checks are together?
    
    ```
    if (validationError.isNone() &&
        frameworkInfo.user() == "root" && !flags.root_submissions) {
    }
    ```



src/master/master.cpp (line 1751)
<https://reviews.apache.org/r/37046/#comment148396>

    Indentation?



src/master/master.cpp (line 1761)
<https://reviews.apache.org/r/37046/#comment148397>

    Mind adding a newline comment between these? I find it pretty dense to read 
when a TODO is right up against the rest of the comment:
    
    ```
            // This could happen if a framework tries to subscribe after
            // its failover timeout has elapsed or it unregistered itself
            // by calling 'stop()' on the scheduler driver.
            //
            // TODO(vinod): Master should persist admitted frameworks to the
            // registry and remove them from it after failover timeout.
    ```



src/master/master.cpp (lines 1849 - 1852)
<https://reviews.apache.org/r/37046/#comment148403>

    Do we want to log differently based on whether the framework id is set?



src/master/master.cpp (line 1890)
<https://reviews.apache.org/r/37046/#comment148406>

    s/failover/force/ ?



src/master/master.cpp (line 1904)
<https://reviews.apache.org/r/37046/#comment148405>

    subscription?



src/master/master.cpp (line 1917)
<https://reviews.apache.org/r/37046/#comment148409>

    Just an observation, it seems not that intuitive that we call 
failoverFramework when the pids are the same, might benefit from a comment.


- Ben Mahler


On Aug. 3, 2015, 7:26 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37046/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2015, 7:26 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Ben Mahler.
> 
> 
> Bugs: MESOS-3182
>     https://issues.apache.org/jira/browse/MESOS-3182
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Merged registerFramework() and reregisterFramework() (and their corresponding 
> continuations) into subscribe() (and _subscribe()).
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
> 
> Diff: https://reviews.apache.org/r/37046/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Reply via email to