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

Ship it!


Looks good given the current Call API. However, per our chat, 'force' doesn't 
make sense for initial SUBSCRIBE calls, I left a comment for how we might want 
to address that. =/


src/master/master.cpp (line 1747)
<https://reviews.apache.org/r/36518/#comment145459>

    Seems like we might want to keep the condition consistent across all of 
these checks (i.e. has_id && id != ""), up to you.
    
    At least, would be nice to add an != operator for FrameworkID vs string.



src/tests/scheduler_tests.cpp (lines 110 - 112)
<https://reviews.apache.org/r/36518/#comment145461>

    Since there's no RESUBSCRIBE, shall we simply call this test 'Subscribe' (I 
noticed there is no Subscribe test currently) and test the full semantics 
within it? Looks like a test of the 'Subscribe' call to me.



src/tests/scheduler_tests.cpp (line 143)
<https://reviews.apache.org/r/36518/#comment145465>

    Hm.. 'force' doesn't make sense for SUBSCRIBE without a framework id.
    
    Seems like either we:
    
    (1) Make 'force' optional, and document that it is only relevant when the 
framework id is set (re-subscription).
    
    (2) Remove 'force' from SUBSCRIBE, add a RESUBSCRIBE with 'force'? =/



src/tests/scheduler_tests.cpp (line 163)
<https://reviews.apache.org/r/36518/#comment145462>

    why the newline here but not above?


- Ben Mahler


On July 15, 2015, 6:40 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36518/
> -----------------------------------------------------------
> 
> (Updated July 15, 2015, 6:40 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3055
>     https://issues.apache.org/jira/browse/MESOS-3055
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In the process of fixing this, added an additional check in 
> Master::registerFramework() that should've been there in the first place. 
> Similar check exists in Master::reregisterFramework().
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
>   src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 
> 
> Diff: https://reviews.apache.org/r/36518/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Reply via email to