----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66311/#review201488 -----------------------------------------------------------
src/resource_provider/manager.cpp Lines 89 (patched) <https://reviews.apache.org/r/66311/#comment282751> This is not used. src/resource_provider/manager.cpp Lines 234-247 (patched) <https://reviews.apache.org/r/66311/#comment282754> Let's move this into `ResourceProviderManagerProcess::initialize()` to keep this running in the actor's context (although I think this is safe because we spawn the process after the constructor finishes). src/resource_provider/manager.cpp Lines 237 (patched) <https://reviews.apache.org/r/66311/#comment282755> `&Self::recover` src/resource_provider/manager.cpp Lines 242 (patched) <https://reviews.apache.org/r/66311/#comment282756> I actually don't know we have `operator<<` defined for a future! Learned something :) src/resource_provider/manager.cpp Lines 264 (patched) <https://reviews.apache.org/r/66311/#comment282758> This means that if the registrar itself takes a while to recover, and during recovery a lot of RPs subscribe to the manager, then all requests (along with potentially huge `ResourceProviderInfo`s) will be stored in the memory until the recovery is finished. I was wondering if we should consider just rejecting all requests before the reccovery is finished, and have a function returning the future for recovery so the caller can do synchronization if they want to. Thoughts? src/resource_provider/manager.cpp Line 304 (original), 346 (patched) <https://reviews.apache.org/r/66311/#comment282757> I might be missing something, but why is `this` necessary? Ditto below. src/resource_provider/registrar.cpp Lines 193-203 (original), 194-203 (patched) <https://reviews.apache.org/r/66311/#comment282753> How about moving this into `GenericRegistrarProcess::initialize()` to start recovery early? If we do this and keep `recovered` (See below) then this function should return ``` recovered->then([=] { return registry.get(); }) ``` src/resource_provider/registrar.cpp Line 205 (original), 205 (patched) <https://reviews.apache.org/r/66311/#comment282737> Why do we need the `undiscardable` here? Could you add some comments? Also, should we prevent the recovery being discarded due to a caller discarding an `apply` call? If yes then we should do ``` return undiscardable(registry.get()).then(... &Self::_apply ...); ``` in the following `apply()` function. src/resource_provider/registrar.cpp Line 216 (original), 216 (patched) <https://reviews.apache.org/r/66311/#comment282749> The overall logic is correct, but it takes a bit of inference to know that overwriting `registry` with a new `Future` in an earlier `_apply` does not affect a later `_apply` that is chained with the overwritten `Future`. So it seems more readible to me if we keep the original `Option<Future<Nothing>> recovered` (or make it just a `Promise<Nothing>`), and chain `_apply` with `recovered` here. src/resource_provider/registrar.cpp Line 250 (original), 250 (patched) <https://reviews.apache.org/r/66311/#comment282748> The overall logic is correct, but it takes a bit of inference to know that the `Future` obtained from `registry.get()` is guaranteed to be ready. So it seems more readible to me if we keep `registry` as an `Option` rather than a `Future`. src/resource_provider/registrar.cpp Line 381 (original), 387 (patched) <https://reviews.apache.org/r/66311/#comment282750> Not yours, but the use of pointer indicates that this `resource_provider::MasterRegistrarProcess` relies on the lifecycle of the backed `master::Registrar` without any guarantee. Let's add some comments about this. - Chun-Hung Hsiao On April 11, 2018, 7:30 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66311/ > ----------------------------------------------------------- > > (Updated April 11, 2018, 7:30 a.m.) > > > Review request for mesos, Jie Yu and Jan Schlicht. > > > Bugs: MESOS-8735 > https://issues.apache.org/jira/browse/MESOS-8735 > > > Repository: mesos > > > Description > ------- > > This patch adjusts the control flow of the resource provider manager > so that we can in the future make use of persisted resource provider > information. > > > Diffs > ----- > > src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a > src/resource_provider/registrar.hpp > 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 > src/resource_provider/registrar.cpp > 92ef9aecb1e93d10f46e53984391558f9901a60b > src/tests/resource_provider_manager_tests.cpp > c52541bf130ccf4795b989b53331176a64a097ea > > > Diff: https://reviews.apache.org/r/66311/diff/4/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
