----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14902/#review28255 -----------------------------------------------------------
This looks great Jie. I would have given it a 'Ship It' except the test ends rather abruptly. See my comment. src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54980> Swap the ordering of these two. ;) src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54993> Let's be more explicit, it will help users find bugs. How about: if (state == ELECTING) { return Future<Option<uint64_t> >::failed("Coordinator already being elected"); } else if (state == ELECTED) { return Future<Option<uint64_t> >::failed("Coordinator already elected"); } else if (... src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54985> How about s/getProposal()/getLastProposal()/. src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54995> How about being explicit like mentioned above? src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54987> Let's help people reading the code and add a comment explaining that it's possible we already tried an election and lost and thus should try at least the proposal number we had before or greater. Seeing std::max begs the question "how else is proposal getting manipulated?". src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54988> Great comment here, let's do something similar above in 'updateProposal'. src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54992> Why not "Coordinator not elected"? src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54991> Why not "Coordinator not elected"? I dont' think we should be revealing our implementation to clients. src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54990> Why not "Coordinator demoted"? src/log/coordinator.cpp <https://reviews.apache.org/r/14902/#comment54994> How about: CHECK(!missing) << "Not expecting local replica to be missing ... fill in rest here"; src/tests/log_tests.cpp <https://reviews.apache.org/r/14902/#comment54996> Add this back given comments above? Knowing why something failed such as the coordinator was demoted can be very helpful for debugging. src/tests/log_tests.cpp <https://reviews.apache.org/r/14902/#comment54998> Hmm, I'm not sure what you're testing. Do you want to look at the result of this? - Benjamin Hindman On Nov. 5, 2013, 12:50 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14902/ > ----------------------------------------------------------- > > (Updated Nov. 5, 2013, 12:50 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-736 > https://issues.apache.org/jira/browse/MESOS-736 > > > Repository: mesos-git > > > Description > ------- > > Libprocessify the coordinator. > > > Diffs > ----- > > src/log/coordinator.hpp 3f6fb7c > src/log/coordinator.cpp 6e6466f > src/tests/log_tests.cpp ff5f86c > > Diff: https://reviews.apache.org/r/14902/diff/ > > > Testing > ------- > > bin/mesos-tests.sh --gtest_filter=*CoordinatorTest*:*LogTest*:*ReplicaTest* > --gtest_repeat=100 > > > Thanks, > > Jie Yu > >
