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