Ben, FYI, your last review is not for the latest version. I addressed all your comments accordingly in the newest version.
- Jie On Mon, Nov 4, 2013 at 12:45 PM, Benjamin Hindman <[email protected]> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14631/ > > It would be great to include some documentation that lists all possible > proposers for the replicas. > > > > src/log/log.hpp<https://reviews.apache.org/r/14631/diff/6/?file=370346#file370346line190> > (Diff > revision 6) > > public: > > 186 > > quorum = _quorum; > > 190 > > replica.reset(new Replica(path)); > > Any reason not to do this in the initializer list above? > > > > src/log/log.hpp<https://reviews.apache.org/r/14631/diff/6/?file=370346#file370346line222> > (Diff > revision 6) > > public: > > 213 > > replica = new Replica(path); > > 217 > > replica.reset(new Replica(path)); > > 214 > > 218 > > 215 > > group = new zookeeper::Group(servers, timeout, znode, auth); > > 219 > > network.reset(new ZooKeeperNetwork(servers, timeout, znode, auth)); > > Any reason not to do these in the initializer list above? I.e., > > group(new zookeeper::Group(servers, timeout, znode, auth)), > quorum(_quorum), > replica(new Replica(path)), > network(new ZooKeeperNetwork(servers, timeout, znode, auth)) > { > > > > src/log/log.hpp<https://reviews.apache.org/r/14631/diff/6/?file=370346#file370346line283> > (Diff > revision 6) > > public: > > 276 > > // For renewing membership. > > 264 > > zookeeper::Group* group; > > 277 > > zookeeper::Group* group; > > How about a bit more context? I.e., "We store a Group instance in order to > continually renew the replicas membership (when using ZooKeeper)." > > > > src/log/network.hpp<https://reviews.apache.org/r/14631/diff/6/?file=370347#file370347line258> > (Diff > revision 6) > > inline void Network::set(const std::set<process::UPID>& pids) > > 258 > > group = new zookeeper::Group(servers, timeout, znode, auth); > > Does it need to be a pointer? Also, can ZooKeeperNetwork be copied? Do we > want it to be copyable? > > > > src/log/replica.hpp<https://reviews.apache.org/r/14631/diff/6/?file=370348#file370348line63> > (Diff > revision 6) > > public: > > 60 > > uint64_t from, uint64_t to) const; > > Let's put each parameter on it's own line. > > > > src/log/replica.hpp<https://reviews.apache.org/r/14631/diff/6/?file=370348#file370348line72> > (Diff > revision 6) > > public: > > 69 > > uint64_t from, uint64_t to) const; > > Each parameter on it's own line please. > > > > src/log/replica.cpp<https://reviews.apache.org/r/14631/diff/6/?file=370349#file370349line540> > (Diff > revision 6) > > Try<Action> LevelDBStorage::read(uint64_t position) > > 536 > > // Last promise made to a coordinator. > > 534 > > // Last promise made to a coordinator. > > s/coordinator/proposer/ > > > > src/messages/log.proto<https://reviews.apache.org/r/14631/diff/6/?file=370350#file370350line22> > (Diff > revision 6) 22 > > // Represents a "promise" that a replica has made to a coordinator. A > > 22 > > // Represents a "promise" that a replica has made to a coordinator. A > > s/to a coordinator// > > > > src/messages/log.proto<https://reviews.apache.org/r/14631/diff/6/?file=370350#file370350line25> > (Diff > revision 6) 25 > > // same coordinator), until a new promise is made to a coordinator > > 25 > > // same coordinator), until a new promise is made to a coordinator > > s/coordinator/proposer/ > > > - Benjamin Hindman > > On November 1st, 2013, 12:18 a.m. UTC, Jie Yu wrote: > Review request for mesos and Benjamin Hindman. > By Jie Yu. > > *Updated Nov. 1, 2013, 12:18 a.m.* > *Bugs: * MESOS-736 <https://issues.apache.org/jira/browse/MESOS-736> > *Repository: * mesos-git > Description > > This is the first patch of a series of patches that implement a catch-up > mechanism for replicated log. See the following ticket for more > details:https://issues.apache.org/jira/browse/MESOS-736 > > Here is a brief summary of this patch: (Sorry for the fact that we are not > able to break it into smaller patches :() > > 1) Pulled the original Coordinator logic out and divides it into several > Paxos phases (see src/log/consensus.hpp). Instead of using a blocking > semantics, we implemented all the logics asynchronously. > > 2) In order to ensure the liveness of a catch-uper, we implemented a retry > logic by bumping the proposal number. This also requires us to slightly > change the existing replica protocol. > > 3) Made the "fill" operation independent of the underlying replica. Instead, > introduced a catchup (see src/log/catchup.hpp) function to make sure the > underlying local replica has learned each write. > > 4) Modified the log tests to adapt to the new semantics (see (3) above) > > This is a joint work with Yan Xu. > > Testing > > bin/mesos-tests.sh --gtest_filter=*CoordinatorTest*:*LogTest*:*ReplicaTest* > --gtest_repeat=100 > > Diffs > > - src/Makefile.am (a11c76b) > - src/log/catchup.hpp (PRE-CREATION) > - src/log/catchup.cpp (PRE-CREATION) > - src/log/consensus.hpp (PRE-CREATION) > - src/log/consensus.cpp (PRE-CREATION) > - src/log/coordinator.hpp (3f6fb7c) > - src/log/coordinator.cpp (6e6466f) > - src/log/log.hpp (77edc7a) > - src/log/network.hpp (d34cf78) > - src/log/replica.hpp (d1f5ead) > - src/log/replica.cpp (59a6ff3) > - src/messages/log.proto (3d5859f) > - src/tests/log_tests.cpp (ff5f86c) > > View Diff <https://reviews.apache.org/r/14631/diff/> >
