> On Oct. 17, 2013, 6:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 2685-2687 > > <https://reviews.apache.org/r/14616/diff/1/?file=364020#file364020line2685> > > > > In the old code, because StatusUpdateManager did not take an optional > > SlaveState, in one case we only recovered the Isolator and in another case > > we recovered both the SUM and the Isolator. > > > > Now that it takes an option, we can always recover both the SUM and the > > Isolator, and if reconnect takes an Option then we can always just call all > > three, leaving only a single return statement! :) > > > > So recoverState() can be _recover(), and reconnect can be __recover(): > > > > We can have a single return inside _recover. There is a bit of > > clunkyness going from Result -> Option but we can make that easier in stout > > :) > > > > recover() { > > async (state::recover) > > .then(defer(self(), &Self::_recover, lambda::_1)); > > } > > > > _recover(Result<SlaveState> state) { > > // Check for state being an error. > > if (state is error) { EXIT(1) << "..." } > > > > if (state is non empty) { > > // Check for SlaveInfo compatibility. > > // Check for recoveryErrors. > > // Recover frameworks and executors. > > } > > > > // The only return statement in _recover! > > // Now recover the SUM, the Isolator, and then complete recovery. > > return statusUpdateManager->recover(metaDir, state) > > .then(defer(isolator, &Isolator::recover, state) > > .then(defer(self(), &Self::__recover, state); > > } > > > > __recover(Option<SlaveState> state) { > > // Monitor the executors and reconnect if desired. > > } > > > > It looks like we don't necessarily need to pass reconnect around since > > it's specified in the flag? > > > > Altogether, we could kill the new helpers and new recoveryState, and > > have a flow of recover->_recover->__recover which may be easier for follow > > and later convert to lambdas if it simplifies things further.
We already have a _recover() which signals the end of recovery. I ended up refactoring it a bit differently based on your suggestions. Let me know what you think. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14616/#review27136 ----------------------------------------------------------- On Oct. 12, 2013, 12:40 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14616/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2013, 12:40 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-732 > https://issues.apache.org/jira/browse/MESOS-732 > > > Repository: mesos-git > > > Description > ------- > > Improvements: > > --> Reading from disk is now done in a separate libprocess (process::async > ftw :)) > > --> Streamlined recovery code. > > > Diffs > ----- > > src/slave/slave.hpp 22fb74b71a0f52d9d67b92ecc286fa8d350e41a4 > src/slave/slave.cpp debb2f4ce05fbfec450197e68bc8a0c78f1d0adf > src/slave/state.cpp 5208e4e8eaaea5aea9e23a6ac7d09822e15433b2 > src/slave/status_update_manager.hpp > 1f55f868548ae79052856e0255097b447ffe7573 > src/slave/status_update_manager.cpp > b6afeb18d3bd97ca4f4238b2ffbf6bd7de8e6888 > src/tests/slave_recovery_tests.cpp 4d262c65043ad86e0851ac816092f6e8b468a114 > > Diff: https://reviews.apache.org/r/14616/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
