----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19762/#review39804 -----------------------------------------------------------
Ship it! Looks good to me, I included a comment below about how we may want to add a TODO to reflect that ultimately accumulators would need to be done a bit differently if we aim to make a generic 'batch operation' abstraction. I would love to see some additional tests now that we're relying on the hashset to make decisions. Namely, have a test that uses a second Registrar to verify that the first Registrar left the 'Registry' protobuf in the correct state. Does that sound good? If so, please follow up in a subsequent change. src/master/registrar.hpp <https://reviews.apache.org/r/19762/#comment72412> We should mention that we'd like to do this because it turns out this idea of "doing batches of operations" is likely to be used elsewhere in the code (potentially when we add repair state, offer reservations, etc). src/master/registrar.hpp <https://reviews.apache.org/r/19762/#comment72411> Let's add to this comment to reflect that the operation is now performed on the 'registry' and the accumulators, which in this case is just 'slaveIDs'. src/master/registrar.cpp <https://reviews.apache.org/r/19762/#comment72436> How about: // Create the 'slaveIds' accumulator. So rather than constructing the 'accumulator' (slaveIds) from 'T' (Registry) once the state has been fetched, we're only materializing accumulators for the purpose of applying operations. That seems fine but it would be good to include a comment that we're doing this because the logic is simpler, is that correct? The alternative is to treat 'T' and the 'accumulators' in the same manner: Option<Variable<Registry> > variable; // T. Option<hashset<SlaveID> > slaveIds; // Accumulator 1. _recover(...) { ... variable = recovery.get(); slaveIds = SlaveIDs(variable.get()); // Imagine a 'SlaveIDs' struct possibly extending from an // 'Accumulator', accumulators would be construct-able from a 'T'. } And the code in update() copies 'Registry' and all accumulators in the same manner: update(...) { // Copy 'T' and 'accumulators'. Registry registry = variable.get().get(); SlaveIDs slaveIds = slaveIds; ... } _update(...) { // All accumulators get re-constructed from the new 'T'. } Of course, this does look to be more complex in that it involves less "local" reasoning, but it offers the opportunity for accumulators to be leveraged *across* batches. This would only be beneficial if copying the accumulator is cheaper than constructing a fresh one. What you have now seems good to me, but let's at least add a TODO on 'Registrar' for including accumulators as template parameters. - Ben Mahler On April 3, 2014, 10:30 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19762/ > ----------------------------------------------------------- > > (Updated April 3, 2014, 10:30 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-1155 > https://issues.apache.org/jira/browse/MESOS-1155 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b > src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177 > src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c > > Diff: https://reviews.apache.org/r/19762/diff/ > > > Testing > ------- > > mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” > –gtest_also_run_disabled_tests > On test machines this reduces the time for applying 20,000 (re)admit > operations from 30secs to < 100ms > > > Thanks, > > Jiang Yan Xu > >
