> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/registrar.cpp > > Lines 193-203 (original), 194-203 (patched) > > <https://reviews.apache.org/r/66311/diff/4/?file=1995451#file1995451line195> > > > > 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(); }) > > ``` > > Benjamin Bannier wrote: > We already require users to `recover` registrars before being able to > perform operations against them (like for other registrars), so I am not > really sure I understand how what you suggest would help. Could you explain? > > Chun-Hung Hsiao wrote: > Oh what I mean is doing the following: > ``` > virtual void initialize() override { > registry = ... // start the recovery. > } > > Future<Registry> recover() { > return registry; > } > ``` > > The second part of my comment above is just illustrating the code if we > keep `registry` as an `Option` and `recovered` as a `Future<Nothing>`.
Thank you, that makes sense. I adjusted the code to track recovery status explicitly (`Future<Nothing> recovered`) which allows us a simple `Option<Registry> registry`. I adjusted the code like that. > On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/registrar.cpp > > Line 205 (original), 205 (patched) > > <https://reviews.apache.org/r/66311/diff/4/?file=1995451#file1995451line207> > > > > 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. > > Benjamin Bannier wrote: > I added a comment and also fixed below `apply` to use `recover()` which > would return the cached result, already guarded by `undiscarable`. > > Chun-Hung Hsiao wrote: > I was condering that we could do `undiscardable` in `apply` so that the > caller can actually discard the recovery if they want to. Not sure if this is > a valid use case though. Please drop it if you don't think so. I think we should allow users to cancel an already pending recovery. I believe this currently not only uninteresting to them, but would also complicate reasoning about internal state and invariants. Marking as fixed. > On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/registrar.cpp > > Line 216 (original), 216 (patched) > > <https://reviews.apache.org/r/66311/diff/4/?file=1995451#file1995451line218> > > > > 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. > > Benjamin Bannier wrote: > I replaced the direct access to `registry` with `recover` here which once > recovered would just serve a cached result. Does that look more reasonable to > you? > > Chun-Hung Hsiao wrote: > The thing that seems unintuitive to me is that `recover()` would return > different futures at different times. May I ask what the reason to make > `registry` a `Future<Registry>` instead of an `Option<Registry>`? That's a very good point. I changed the code to always return the same registry now (we'll hold an `Option<Registry>` instead of a `Future<Registry>` now). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66311/#review201488 ----------------------------------------------------------- On April 20, 2018, 12:23 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66311/ > ----------------------------------------------------------- > > (Updated April 20, 2018, 12:23 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, 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. While this patch sets up the needed flow, it does not > implement recovery logic, yet. > > > 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/6/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
