> On March 7, 2014, 7:45 p.m., Ben Mahler wrote: > > src/log/recover.cpp, lines 387-389 > > <https://reviews.apache.org/r/18584/diff/4/?file=513745#file513745line387> > > > > 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.
Okay, after chatting with Jie, this may not work so well with r/18600. So changing s/checkRecoverProtocol/_recover/ is more clear about the fact that recovery is taking place inside this method. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18584/#review36553 ----------------------------------------------------------- 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 > >
