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


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line1>
> >
> >     This patch should contain a test of introduced behavior.

Test done later in chain. I prefer splitting the fix that is targeting for backporting and
the test, so the test needs not to be backported. Or do you think we should backport the test
as well?


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2702 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line2734>
> >
> >     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.

`call` should be a darn simple wrapper. The call site has more context about what to check
and how to react. In this example, the lack of this capability shouldn't result in an error.


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3202-3203 (original), 3194-3196 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line3227>
> >
> >     Let's use a `switch` now that we have three cases here.

Good point. These checks actually don't give us much value and I was thinking about removing
them. Using a `switch` and moving it into the continuation instead, could enable the compiler
to check if we forget to clear some field in the future.


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3237-3239 (original), 3235-3237 (patched)
> > <https://reviews.apache.org/r/70314/diff/2/?file=2134394#file2134394line3269>
> >
> >     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";

We usually quote resoucre in the codebase.


- Chun-Hung


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


On March 27, 2019, 3: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, 3: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