----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68763/#review208767 -----------------------------------------------------------
src/resource_provider/daemon.cpp Line 115 (original), 115 (patched) <https://reviews.apache.org/r/68763/#comment292929> I believe using the `None` state to denote `is being removed` is hard to follow below. I'd much prefer an explicit field, even though that would mean we'd need to maintain consistent state. src/resource_provider/daemon.cpp Lines 310-314 (patched) <https://reviews.apache.org/r/68763/#comment292930> This is a pretty bare-bones API. I believe if we'd instead schedule removal once the launch has finished, we'd not only remove the burden to retry from the caller, but probably also make this function safer to use. This would likely require us to maintain `Promise<Owned<LocalResourceProvider>>` instead. We should probably move this into below `then` continuation. src/resource_provider/daemon.cpp Line 290 (original), 319 (patched) <https://reviews.apache.org/r/68763/#comment292931> Not sure how this will look like once we handle agent failover, but it seems this removal should only happen after successful `stop`. That way we might e.g., be able to maintain consistent state (config exists <==> container running). src/resource_provider/daemon.cpp Lines 489 (patched) <https://reviews.apache.org/r/68763/#comment292927> Let's not enforce this being set here; just comparing an `Option<UUID>` with an `UUID` is fine below. src/resource_provider/daemon.cpp Lines 498 (patched) <https://reviews.apache.org/r/68763/#comment292928> Do we need to check this precondition here? Assigning over an `Owned` would be fine and not leak. Let's at least add a comment why this is needed otherwise. - Benjamin Bannier On Sept. 19, 2018, 7:14 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68763/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2018, 7:14 a.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-9228 > https://issues.apache.org/jira/browse/MESOS-9228 > > > Repository: mesos > > > Description > ------- > > This patch introduces a `LocalResourceProvider::stop` function to > perform RP-specific stop procedures. SLRP uses this function to kill > plugin container before its config is removed. > > > Diffs > ----- > > src/resource_provider/daemon.cpp 0a76cc6d1d34413674d1af1aa514679a4d2b7b55 > src/resource_provider/local.hpp 20bcc78d3fe847e03526fa59116bdbac92ec1e29 > src/resource_provider/storage/provider.hpp > 5a371b19289c6e39fedd4cda65fa8be432d095e6 > src/resource_provider/storage/provider.cpp > 6475f653263337c381b6080695d09c49e5ea8fcf > > > Diff: https://reviews.apache.org/r/68763/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
