> On Feb. 18, 2014, 6:46 p.m., Vinod Kone wrote: > > src/master/registry.proto, lines 27-28 > > <https://reviews.apache.org/r/18158/diff/1/?file=486550#file486550line27> > > > > Why not > > > > repeated<Slave> slaves = 2; ? > > > > I think that would make some of the code in registry.cpp more readable. > > > > For example, > > > > s/registry.slaves.slaves().size()/registry.slaves().size()/ > > > > > > Ben Mahler wrote: > This is a good point, the reason I didn't do this was because we were > originally storing a "slaves" key in the Registry state to atomically modify > slaves. Now we use a single "registry" key for all Registry information and I > agree the 'Slaves' wrapper ends up being a bit clunky in the C++ code. > > There are a few reasons I kept the 'Slaves' wrapper: > > 1. In the C++, if one wants to pass around the slaves of the Registry, > one would have to use a 'RepeatedPtrField<Slave>' rather than just passing > around 'Slaves', this could end up being pretty clunky as well. > > 2. Less realistically, as the Registry grows, we may want to abandon > the single "registry" key model and store separate keys for "slaves", > "master", etc. Keeping a 'Slaves' wrapper protobuf makes this much easier to > do, since the protobuf message for slaves would not change. > > I've left your issue open, let me know what you think!
I agree with both the points but (1) is not an issue today (AFACIT, Slaves* is never being passed around) and (2) doesn't seem to be realistic future direction considering atomicity requirements? Should we keep it simple for now and revisit if the need arises? I understand changing protobufs is hard, so want to make sure our arguments for this decision are reasonable. For example, do you imagine something that ties all the slaves that is going to go into the Slaves protobuf? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18158/#review34731 ----------------------------------------------------------- On Feb. 20, 2014, 4:16 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18158/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2014, 4:16 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Bugs: MESOS-764 > https://issues.apache.org/jira/browse/MESOS-764 > > > Repository: mesos-git > > > Description > ------- > > The registrar now uses a single Variable for the Registry, as outlined within: > https://cwiki.apache.org/confluence/display/MESOS/Registrar+Design+Document > > As a heads up, there are a number of changes / fixes that will follow this as > well, I am attempting to break these changes apart. > > > Diffs > ----- > > src/master/registrar.cpp 915885a160f790399e8185c28c6e6555af1ee76e > src/master/registry.proto bd850997c57153cdfc0c39d0dae7d2b802034aa3 > src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243 > > Diff: https://reviews.apache.org/r/18158/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
