----------------------------------------------------------- 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 > >
