You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/10/14 20:30:45 UTC

Re: Review Request 14383: Added the registrar.

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

Ship it!


Awesome, love the batching stuff you added here!

I added comments, some of which I will need fixed to integrate the Registrar into the Master. However, I'm giving you a ship it regardless depending on whether you would like to commit and leave the comments for me or make adjustments in this review.


src/master/registrar.hpp
<https://reviews.apache.org/r/14383/#comment52394>

    We also need something like:
    
    Future<list<SlaveInfo>> retrieve();
    
    That returns the full list of slaves in order for the master to recover state after a failover. But please think of a better name than "retrieve". :)



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52396>

    Seems a bit odd to have a struct subclass a class?
    
    Also, I'm curious why you chose inheritance here over composition?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52397>

    Seems expensive to iterate over the entire protobuf (thousands of slaves) to decide whether we can add the slave. Any chance of adding a lookup?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52398>

    CopyFrom seems more appropriate?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52399>

    "Even though" "state,"



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52400>

    Again seems very expensive to go over all the slaves here, ditto for the other Mutations. Thoughts on this?
    
    Were you missing a break statement in the loop? Also, you could remove the need for the 'found' boolean if you like:
    
    foreach (const registry::Slave& slave, slaves.slaves()) {
      if (slave.info().id() == info.id()) {
        set(true);
        return slaves;
      }
    }
    
    set(false);
    return slaves;



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52401>

    This could just return here and eliminate the need for the 'removed' bool.



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52518>

    It looks like state->fetch can be called twice, if say:
    
    recover() is called
    recover() is called again (prior to _recover completing)
    now state->fetch was called twice?
    
    Should this set slaves.updating = true?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52519>

    It seems as though the master may want to explicitly call registrar->recover, which returns a list of SlaveInfos. This would allow the master to directly know that recovery failed, thoughts?
    
    The implicit infinite retry is going to cause all admit/readmit/remove calls to take forever if something is wrong, no?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52520>

    I really like the batching technique here!



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52521>

    Maybe a little note about why we can safely apply the remaining mutations?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52523>

    I'm confused here, looking at state/protobuf.hpp it looks like Variable<T>::mutate does not return a Try?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52526>

    Is "version mismatch" the UUID not matching in Storage::set? Are you expecting that to be possible, or just being defensive?



src/master/registrar.cpp
<https://reviews.apache.org/r/14383/#comment52524>

    Would be nice to print the failure string :)



src/tests/registrar_tests.cpp
<https://reviews.apache.org/r/14383/#comment52531>

    Would love if these tests were templated to use both LevelDBStorage and ZooKeeperStorage with an in-memory store, since the latter is what will be run in production environments :)


- Ben Mahler


On Sept. 28, 2013, 12:32 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14383/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2013, 12:32 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ee336130ad93d8b524c841f75be36f00d4a2b147 
>   src/master/registrar.hpp PRE-CREATION 
>   src/master/registrar.cpp PRE-CREATION 
>   src/master/registry.proto 877bfa1465371f035f5d31606554415146d0c307 
>   src/tests/registrar_tests.cpp PRE-CREATION 
>   src/tests/state_tests.cpp f39dee549ccd959f451bdfedba74837934376443 
> 
> Diff: https://reviews.apache.org/r/14383/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 14383: Added the registrar.

Posted by Ben Mahler <be...@gmail.com>.


> On Oct. 14, 2013, 6:30 p.m., Ben Mahler wrote:
> > Awesome, love the batching stuff you added here!
> > 
> > I added comments, some of which I will need fixed to integrate the Registrar into the Master. However, I'm giving you a ship it regardless depending on whether you would like to commit and leave the comments for me or make adjustments in this review.

I've committed this for you as discussed and I'll take up the comments in a subsequent review, can you mark as submitted?


- Ben


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


On Sept. 28, 2013, 12:32 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14383/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2013, 12:32 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ee336130ad93d8b524c841f75be36f00d4a2b147 
>   src/master/registrar.hpp PRE-CREATION 
>   src/master/registrar.cpp PRE-CREATION 
>   src/master/registry.proto 877bfa1465371f035f5d31606554415146d0c307 
>   src/tests/registrar_tests.cpp PRE-CREATION 
>   src/tests/state_tests.cpp f39dee549ccd959f451bdfedba74837934376443 
> 
> Diff: https://reviews.apache.org/r/14383/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>