> On April 19, 2018, 6 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?
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>`.
> On April 19, 2018, 6 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`.
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.
> On April 19, 2018, 6 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?
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>`?
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/#review201488
-----------------------------------------------------------
On April 19, 2018, 12:29 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 12:29 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.
>
>
> 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/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
