----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18584/#review36553 -----------------------------------------------------------
Looks good, if you end up changing the chain in RecoverProcess we should do another pass, let me know which approach you think is nicer. src/log/recover.cpp <https://reviews.apache.org/r/18584/#comment67556> CHECK_SOME src/log/recover.cpp <https://reviews.apache.org/r/18584/#comment67569> I'm wondering if we can flatten the continuation chain here: Current: recover runRecoverProtocol checkRecoverProtocol updateReplicaStatus(RECOVERING) catchup getOwnership updateReplicaStatus(VOTING) It's a bit tricky because "checking the recover protocol" implies "updating the replica state" and doing "catchup" when necessary. A flatter approach could be: recover runRecoverProtocol checkRecoverProtocol updateReplicaStatus(RECOVERING) catchup getOwnership updateReplicaStatus(VOTING) The unfortunate part of this alternative approach is that we can no longer pass in 'begin' and 'end' to catchup. We could store an Option<Interval> to capture the catchup interval in checkRecoverProtocol() and CHECK_SOME(interval) in catchup(). The chain would then look like this: recover() { if (status == Metadata::VOTING) { // No need to do recovery. return Nothing(); } else { return runRecoverProtocol(quorum, network) .then(defer(self(), &Self::checkRecoverProtocol, lambda::_1)) .then(defer(self(), &Self::updateReplicaStatus, Metadata::RECOVERING)) .then(defer(self(), &Self::catchup)) .then(defer(self(), &Self::updateReplicaStatus, Metadata::VOTING)); } } And catchup takes no arguments: Future<Nothing> catchup() { CHECK_SOME(interval); CHECK_LT(interval.get().lower(), interval.get().upper()); LOG(INFO) << "Starting catch-up from position [" << interval.get().lower() << " to " << interval.get().upper() << ")"; IntervalSet<uint64_t> positions; positions += interval.get(); ... } The flatter style might be a bit clearer since we can see the replica status changes in the outer change, and "checking" the recover protocol result is now a pure "check" instead of a "check" and a replica status change. Now, catchup() also doesn't perform an implicit status update change. On the other hand, having to store the 'interval' is unfortunate. I'll leave this up to you, since both approaches have their tradeoffs. src/log/recover.cpp <https://reviews.apache.org/r/18584/#comment67560> Looks like this will fit on one line now? - Ben Mahler On March 7, 2014, 5:20 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18584/ > ----------------------------------------------------------- > > (Updated March 7, 2014, 5:20 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-984 > https://issues.apache.org/jira/browse/MESOS-984 > > > Repository: mesos-git > > > Description > ------- > > Previously, we use onAny to connect most of the steps. As a result, we see a > lot of the error checking code like the following: > > void checked(const Future<Metadata::Status>& future) > { > if (!future.isReady()) { > promise.fail( > future.isFailed() ? > "Failed to get replica status: " + future.failure() : > "Not expecting discarded future"); > terminate(self()); > return; > } > ... > } > > Another drawback is: discarding becomes difficult and hard to reason. > > Now, I cleaned up the code to use the continuation style (i.e., chaining > using Future.then). The code becomes much more clear now! > > Also, I factored out the recover protocol part from the log recover process > to make the log recover process less cumbersome. > > This patch does not change any semantics, but is for MESOS-984 (log auto > initialization for registrar). Will follow up with a patch to integrate the > log auto initialization based on this patch. > > > Diffs > ----- > > src/log/recover.cpp e611a4e > > Diff: https://reviews.apache.org/r/18584/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
