mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 68763: WIP: Stopped resource providers when removing resource provider configs.
Date Wed, 19 Sep 2018 15:35:00 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68763/#review208767
-----------------------------------------------------------




src/resource_provider/daemon.cpp
Line 115 (original), 115 (patched)
<https://reviews.apache.org/r/68763/#comment292929>

    I believe using the `None` state to denote `is being removed` is hard to follow below.
I'd much prefer an explicit field, even though that would mean we'd need to maintain consistent
state.



src/resource_provider/daemon.cpp
Lines 310-314 (patched)
<https://reviews.apache.org/r/68763/#comment292930>

    This is a pretty bare-bones API. I believe if we'd instead schedule removal once the launch
has finished, we'd not only remove the burden to retry from the caller, but probably also
make this function safer to use.
    
    This would likely require us to maintain `Promise<Owned<LocalResourceProvider>>`
instead.
    
    We should probably move this into below `then` continuation.



src/resource_provider/daemon.cpp
Line 290 (original), 319 (patched)
<https://reviews.apache.org/r/68763/#comment292931>

    Not sure how this will look like once we handle agent failover, but it seems this removal
should only happen after successful `stop`. That way we might e.g., be able to maintain consistent
state (config exists <==> container running).



src/resource_provider/daemon.cpp
Lines 489 (patched)
<https://reviews.apache.org/r/68763/#comment292927>

    Let's not enforce this being set here; just comparing an `Option<UUID>` with an
`UUID` is fine below.



src/resource_provider/daemon.cpp
Lines 498 (patched)
<https://reviews.apache.org/r/68763/#comment292928>

    Do we need to check this precondition here? Assigning over an `Owned` would be fine and
not leak. Let's at least add a comment why this is needed otherwise.


- Benjamin Bannier


On Sept. 19, 2018, 7:14 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68763/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2018, 7:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9228
>     https://issues.apache.org/jira/browse/MESOS-9228
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch introduces a `LocalResourceProvider::stop` function to
> perform RP-specific stop procedures. SLRP uses this function to kill
> plugin container before its config is removed.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/daemon.cpp 0a76cc6d1d34413674d1af1aa514679a4d2b7b55 
>   src/resource_provider/local.hpp 20bcc78d3fe847e03526fa59116bdbac92ec1e29 
>   src/resource_provider/storage/provider.hpp 5a371b19289c6e39fedd4cda65fa8be432d095e6

>   src/resource_provider/storage/provider.cpp 6475f653263337c381b6080695d09c49e5ea8fcf

> 
> 
> Diff: https://reviews.apache.org/r/68763/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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