mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66311: Implement recovery of resource provider manager.
Date Thu, 19 Apr 2018 06:00:18 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message