----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70215/#review213981 -----------------------------------------------------------
Fix it, then Ship it! src/resource_provider/storage/provider.cpp Line 1637 (original), 1569 (patched) <https://reviews.apache.org/r/70215/#comment300124> We could move these right after `StorageLocalResourceProviderProcess::attachVolume` like we also do in e.g., `src/master/master.cpp`. Here and below. OTOH, this code is going away soon, so not marking this as an issue. src/resource_provider/storage/provider.cpp Line 1640 (original), 1572 (patched) <https://reviews.apache.org/r/70215/#comment300125> This was here before and seems to work, but I wonder if it wouldn't be easier to follow if we'd return a `Failure` here instead as it would be immediately clear that the code would be safe. Also below. src/resource_provider/storage/provider.cpp Line 1670 (original), 1620 (patched) <https://reviews.apache.org/r/70215/#comment300134> Do we need to `CHECK`? Or can the volume legitimately go away while we `defer` and should return instead here? I'd in general prefer returning something over a `CHECK` as it introduces less coupling. src/resource_provider/storage/provider.cpp Line 1776 (original), 1754 (patched) <https://reviews.apache.org/r/70215/#comment300135> Ditto. src/resource_provider/storage/provider.cpp Line 1899 (original), 1896 (patched) <https://reviews.apache.org/r/70215/#comment300136> Ditto. - Benjamin Bannier On March 22, 2019, 7:18 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70215/ > ----------------------------------------------------------- > > (Updated March 22, 2019, 7:18 a.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-9622 > https://issues.apache.org/jira/browse/MESOS-9622 > > > Repository: mesos > > > Description > ------- > > This patch introduces methods for volume attaching and publishing that > conform to `VolumeManager`'s public interface in SLRP, and cleans up > SLRP based on these functions. They will be moved out from SLRP to v0 > `VolumeManager` later. > > Volume attaching and publishing are implemented with internal helpers, > each individually deals with one steady and two transient states, and > makes a proper CSI call to achieve its goal state. If the given volume > is not in the designed state, it would recursively call other helpers > to transition the volume to the designed state first. > > These helper functions are serialized through the volume's own sequence > to avoid racing operations on the same volume. > > > Diffs > ----- > > src/resource_provider/storage/provider.cpp > fea623c292158deb1b4b4b9ab1ac208031471519 > src/resource_provider/storage/provider_process.hpp > a5536b3d735e01eb1c4dc52d0602d973155f3c93 > > > Diff: https://reviews.apache.org/r/70215/diff/4/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
