You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2018/04/20 10:23:22 UTC
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
-----------------------------------------------------------
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.
Changes
-------
Addressed remaining issues from Chun.
Summary (updated)
-----------------
Set up recovery code paths of resource provider manager.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/66311/diff/5-6/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 23, 2018, 2:44 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 61 (patched)
> > <https://reviews.apache.org/r/66311/diff/6/?file=2007714#file2007714line61>
> >
> > This is not used here but I see it being used in r/66545 so just leave a comment.
Moved it there.
> On April 23, 2018, 2:44 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 174 (original), 179 (patched)
> > <https://reviews.apache.org/r/66311/diff/6/?file=2007716#file2007716line181>
> >
> > Actually, do we really need this? It seems that we can always get `registry` from `variable`?
I just adopted it from the master registrar, and you are right in that it is not needed. On top of that it also introduces a lot of complicated state coupling between `variable` and `registry`, so removing it is a great suggestion. Code adjusted like suggested.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/#review201709
-----------------------------------------------------------
On April 23, 2018, 1:19 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> -----------------------------------------------------------
>
> (Updated April 23, 2018, 1:19 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/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/#review201709
-----------------------------------------------------------
src/resource_provider/manager.cpp
Lines 61 (patched)
<https://reviews.apache.org/r/66311/#comment283260>
This is not used here but I see it being used in r/66545 so just leave a comment.
src/resource_provider/registrar.cpp
Line 174 (original), 179 (patched)
<https://reviews.apache.org/r/66311/#comment283262>
Actually, do we really need this? It seems that we can always get `registry` from `variable`?
src/resource_provider/registrar.cpp
Lines 216 (patched)
<https://reviews.apache.org/r/66311/#comment283261>
`return undiscardable(recovered).then(...)`?
- Chun-Hung Hsiao
On April 20, 2018, 10:23 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> -----------------------------------------------------------
>
> (Updated April 20, 2018, 10:23 a.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
>
>
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/#review201867
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On April 24, 2018, 7:28 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> -----------------------------------------------------------
>
> (Updated April 24, 2018, 7:28 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/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/
-----------------------------------------------------------
(Updated April 26, 2018, 2:39 p.m.)
Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
Changes
-------
Deferred function accessing registrar actor state.
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 (updated)
-----
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/10/
Changes: https://reviews.apache.org/r/66311/diff/9-10/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/
-----------------------------------------------------------
(Updated April 24, 2018, 9:28 p.m.)
Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed additional comments from Chun.
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 (updated)
-----
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/8/
Changes: https://reviews.apache.org/r/66311/diff/7-8/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 23, 2018, 9:43 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Line 222 (original), 253-259 (patched)
> > <https://reviews.apache.org/r/66311/diff/7/?file=2009012#file2009012line254>
> >
> > If this is not called elsewhere, how about not exposing this as a function but put in it `initialize()`?
This is a private function which encapsulates a single aspect, so I feel keeping it would be worthwile. We already use it in `initialize` where it helps improve readability.
Let's keep it, dropping.
> On April 23, 2018, 9:43 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 266-270 (patched)
> > <https://reviews.apache.org/r/66311/diff/7/?file=2009012#file2009012line267>
> >
> > ```
> > // ... actor's indox, which could become large if a big ...
> > ^^^^^^^ ^^^^^^^ ^^
> > ```
> > I'm not a native speaker though, so please drop this if I'm incorrect ;)
Adopted suggestion, dropped _if_ :P
> On April 23, 2018, 9:43 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Lines 199-205 (original), 204-208 (patched)
> > <https://reviews.apache.org/r/66311/diff/7/?file=2009014#file2009014line207>
> >
> > I just realized that I missed one thing here: if `recover()` is called before `recovered` is assigned, it will return a future that is forever pending. We can do one of the followings to resolve this:
> >
> > 1. Revert `recovered` back to an `Option<Future<Nothing>>`, move this assignment back to `recover()` and guard it with `if (recovered.isNone())`, then call `recover()` here.
> >
> >
> > 2. Make `recovered` a `Promise<Nothing>` as we do in `ResourceProviderManagerProcess` above, the do
> > ```
> > recovered.associate(state.fetch<Registry>(NAME).then(...));
> > ```
> >
> > 3. Don't do anything. Since this is an internal class and the exposed `GenericRegistrar` always dispatch `recover()` to this actor, `initialize()` is guaranteed to always called before `recover()`. This relies on a convention between this actor implementation and the `GenericRegistrar` class.
> >
> > I think either of the 3 is okay. Personally I would prefer adjusting either this class or the `ResourceProviderManagerProcess` class to make the two consistent though.
Thanks for the careful reading. In general we seem to be fine with (3) since we use actors and their wrappers in a pretty canoncial way. I nevertheless fixed the code to use (2) semantics since it is safer in general and (in contrast with (1)) does not require assertions in the code.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/#review201754
-----------------------------------------------------------
On April 24, 2018, 9:28 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> -----------------------------------------------------------
>
> (Updated April 24, 2018, 9:28 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/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/#review201754
-----------------------------------------------------------
src/resource_provider/manager.cpp
Lines 244-249 (patched)
<https://reviews.apache.org/r/66311/#comment283341>
Nit: Please fix the indentation.
src/resource_provider/manager.cpp
Line 222 (original), 253-259 (patched)
<https://reviews.apache.org/r/66311/#comment283345>
If this is not called elsewhere, how about not exposing this as a function but put in it `initialize()`?
src/resource_provider/manager.cpp
Lines 266-270 (patched)
<https://reviews.apache.org/r/66311/#comment283343>
```
// ... actor's indox, which could become large if a big ...
^^^^^^^ ^^^^^^^ ^^
```
I'm not a native speaker though, so please drop this if I'm incorrect ;)
src/resource_provider/registrar.cpp
Lines 199-205 (original), 204-208 (patched)
<https://reviews.apache.org/r/66311/#comment283346>
I just realized that I missed one thing here: if `recover()` is called before `recovered` is assigned, it will return a future that is forever pending. We can do one of the followings to resolve this:
1. Revert `recovered` back to an `Option<Future<Nothing>>`, move this assignment back to `recover()` and guard it with `if (recovered.isNone())`, then call `recover()` here.
2. Make `recovered` a `Promise<Nothing>` as we do in `ResourceProviderManagerProcess` above, the do
```
recovered.associate(state.fetch<Registry>(NAME).then(...));
```
3. Don't do anything. Since this is an internal class and the exposed `GenericRegistrar` always dispatch `recover()` to this actor, `initialize()` is guaranteed to always called before `recover()`. This relies on a convention between this actor implementation and the `GenericRegistrar` class.
I think either of the 3 is okay. Personally I would prefer adjusting either this class or the `ResourceProviderManagerProcess` class to make the two consistent though.
src/resource_provider/registrar.cpp
Line 215 (original), 226 (patched)
<https://reviews.apache.org/r/66311/#comment283347>
`undiscardable(recovered).then(...)` or `recover().then(...)`.
src/resource_provider/registrar.cpp
Line 284 (original), 292 (patched)
<https://reviews.apache.org/r/66311/#comment283348>
This parameter is no longer needed.
- Chun-Hung Hsiao
On April 23, 2018, 11:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> -----------------------------------------------------------
>
> (Updated April 23, 2018, 11:19 a.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/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66311: Set up recovery code paths of resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66311/
-----------------------------------------------------------
(Updated April 23, 2018, 1:19 p.m.)
Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
Changes
-------
Rebased code since `registrar` is now guaranteed to be not null.
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 (updated)
-----
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/7/
Changes: https://reviews.apache.org/r/66311/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier