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 70314: Supported destroying preprovisioned CSI volumes in SLRP.
Date Wed, 27 Mar 2019 09:51:43 GMT

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Line 1 (original), 1 (patched)
<https://reviews.apache.org/r/70314/#comment300260>

    This patch should contain a test of introduced behavior.



src/resource_provider/storage/provider.cpp
Lines 2646 (patched)
<https://reviews.apache.org/r/70314/#comment300256>

    Add a comment here documenting what this branch models.



src/resource_provider/storage/provider.cpp
Lines 2702 (patched)
<https://reviews.apache.org/r/70314/#comment300258>

    All this local checking of capabilities at call sites instead of in `call` is getting
cumbersome and error-prone. Would be great to clean that up eventually by moving it to a single
place.



src/resource_provider/storage/provider.cpp
Lines 2725 (patched)
<https://reviews.apache.org/r/70314/#comment300257>

    Great, should have been here all along.



src/resource_provider/storage/provider.cpp
Lines 3202-3203 (original), 3194-3196 (patched)
<https://reviews.apache.org/r/70314/#comment300259>

    Let's use a `switch` now that we have three cases here.



src/resource_provider/storage/provider.cpp
Lines 3237-3239 (original), 3235-3237 (patched)
<https://reviews.apache.org/r/70314/#comment300255>

    No quotes around `resource`; let's also fix the wrapping so that related entities are
on the same line (`"resource provider"` and `info.id()`; `"resource"` and `resource`; opening
and closing quotes).
    ```
    LOG(INFO)
      << "Reconciling storage pools for resource provider " << info.id()
      << " after resource '" << resource << "' has been freed";


- Benjamin Bannier


On March 27, 2019, 4:45 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70314/
> -----------------------------------------------------------
> 
> (Updated March 27, 2019, 4:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9540
>     https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
> If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
> capability, this operation will be a no-op; otherwise the underlying CSI
> volume will be deprovisioned.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/provider.cpp 2711503cdb58cb9b34af8c9fad0908c5f788a2af

> 
> 
> Diff: https://reviews.apache.org/r/70314/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> The new code path is tested later in this chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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