> On Oct. 23, 2015, 11:52 p.m., Jie Yu wrote: > > src/tests/log_tests.cpp, line 708 > > <https://reviews.apache.org/r/39325/diff/5/?file=1102092#file1102092line708> > > > > No snake_case please. Also, we try not to use tailing underscore for > > member fields if possible.
Re: underscores, the style guide says "Prefer trailing underscores for use as member fields (but not required)." -- is that incorrect? > On Oct. 23, 2015, 11:52 p.m., Jie Yu wrote: > > src/log/consensus.cpp, line 334 > > <https://reviews.apache.org/r/39325/diff/5/?file=1102087#file1102087line334> > > > > 'type' is optional. Do you need to check `has_type()`? As written, I think it is actually safe (type() should not equal PromiseResponse::IGNORE if it isn't set), but you're right that checking for it explicitly is probably safer and more readable. > On Oct. 23, 2015, 11:52 p.m., Jie Yu wrote: > > src/log/consensus.cpp, line 356 > > <https://reviews.apache.org/r/39325/diff/5/?file=1102087#file1102087line356> > > > > This is a little hard to read. Can you introduce a small helper: > > ``` > > bool isReject(const Response& response) > > { > > if (response.has_type()) { > > // new replica > > return response.type() == PromiseResponse::REJECT; > > } else { > > // old replica > > return !response.okay(); > > } > > } > > ``` > > > > To be consistent, you might wanna introduce `isIgnore` as well. I didn't introduce isIgnore(): since we already have 2 utility functions (for WriteResponse and PromiseResponse), requiring 4 functions seemed like it would be a bit much. > On Oct. 23, 2015, 11:52 p.m., Jie Yu wrote: > > src/messages/log.proto, line 142 > > <https://reviews.apache.org/r/39325/diff/5/?file=1102091#file1102091line142> > > > > s/old master/old coordinator/ Isn't "master" accurate, since a non-coordinator might try to pass a promise (e.g., during catchup)? - Neil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39325/#review103873 ----------------------------------------------------------- On Oct. 29, 2015, 4:50 a.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39325/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2015, 4:50 a.m.) > > > Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen. > > > Bugs: MESOS-3280 > https://issues.apache.org/jira/browse/MESOS-3280 > > > Repository: mesos > > > Description > ------- > > MESOS-3280. The basic problem is that replicas silently ignore inbound Promise > and Write requests if they have not finished the recovery protocol yet > (because > they can't safely vote on such requests). Hence, if we try to do a Paxos round > while a quorum of nodes have not finished recovering, the Paxos round will > never > complete. In particular, this might happen during coordinator election: > coordinator election (which is implemented as performing a full Paxos round) > starts as soon as the candidate coordinator replica has finished the recovery > protocol. If several nodes start concurrently, a quorum of those nodes might > still be executing the recovery protocol, and hence the coordinator will never > be elected. > > To address this, add "ignored" responses to the Promise and Write > sub-protocols: > if a proposer sees a quorum of "ignored" responses to a promise or write > request > it has issued, it knows the request will never succeed. When used for > coordinator election, the current coding will retry immediately (without a > backoff). > > Note that replicas will still silently drop promise/write requests if another > kind of problem occurs (e.g., an I/O error prevents reading/writing log > data). We might consider changing this, although it will require some thought: > e.g., if a replica's disk is broken, sending an "ignored" message on every > request might flood the network. > > > Diffs > ----- > > src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 > src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 > src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 > src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 > src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 > src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 > > Diff: https://reviews.apache.org/r/39325/diff/ > > > Testing > ------- > > "make check" passes, including a new test that uses a newly constructed mock > to ensure we're testing the message schedule described above. > > I also wrote a script stops and starts mesos-master in a loop, removing the > replicated log each time. Without the patch, this occasionally fails with a > "registry fetch" timeout; with the patch, you can observe several scenarios > where coordinator election is reborted and retried because a quorum of > ignored responses is seen. Note that in some cases, we need to retry > coordinator election up to ~70 times (!), because we don't currently use a > backoff; that should probably be fixed, per comments above. But the important > point is that election eventually succeeds and we don't hang. > > > Thanks, > > Neil Conway > >
