----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70214/#review213804 -----------------------------------------------------------
This review does seem to do exactly what https://reviews.apache.org/r/70213/ started, but not actually implement any functionality either. Squashing them both wouldn't be confusing I think. src/csi/v0_volume_manager.hpp Lines 57 (patched) <https://reviews.apache.org/r/70214/#comment299832> Did you intend to take a ref here, or `move` when initializing the member below? The former would be consistent to the treatment of other arguments. src/csi/v0_volume_manager.hpp Lines 100 (patched) <https://reviews.apache.org/r/70214/#comment299852> Does it make sense to move this into the process? It would lead to simpler dispatch in the wrapper around the process, and probably be more in line with the way we usually lay this out. src/csi/v0_volume_manager.cpp Lines 70 (patched) <https://reviews.apache.org/r/70214/#comment299844> Let's add a log string here as this isn't an internal invariant. Is this something which should have been defined as a precondition in the interface? src/csi/v0_volume_manager.cpp Lines 76 (patched) <https://reviews.apache.org/r/70214/#comment299853> Shouldn't we be able to dispatch to the process for all of these already like we already do for some methods below? I'd say either dispatch to all existing process methods, or don't even bother declaring unused methods in the process. src/csi/v0_volume_manager.cpp Lines 161 (patched) <https://reviews.apache.org/r/70214/#comment299845> The `CHECK_NOTNULL` seems redundant here, but doesn't hurt. src/csi/v0_volume_manager.cpp Lines 181 (patched) <https://reviews.apache.org/r/70214/#comment299846> We usually wrap before `.then` (and probably should update our `clang-format` fork to do it as well). src/csi/v0_volume_manager.cpp Lines 189 (patched) <https://reviews.apache.org/r/70214/#comment299847> Ditto. src/csi/v0_volume_manager.cpp Lines 218 (patched) <https://reviews.apache.org/r/70214/#comment299848> Ditto. src/csi/v0_volume_manager.cpp Lines 229 (patched) <https://reviews.apache.org/r/70214/#comment299849> Ditto. src/csi/v0_volume_manager.cpp Lines 243 (patched) <https://reviews.apache.org/r/70214/#comment299850> Ditto. src/csi/v0_volume_manager.cpp Lines 257 (patched) <https://reviews.apache.org/r/70214/#comment299851> Ditto. src/csi/v0_volume_manager_process.hpp Lines 49 (patched) <https://reviews.apache.org/r/70214/#comment299843> Why can't we define this in `src/csi/v0_volume_manager.hpp` as we usually do? src/csi/v0_volume_manager_process.hpp Lines 98 (patched) <https://reviews.apache.org/r/70214/#comment299854> Let's make this class non-copyable referencing MESOS-5122. src/csi/volume_manager.cpp Line 43 (original), 49 (patched) <https://reviews.apache.org/r/70214/#comment299842> Same question as on previous review: how do you plan to do version-dependent dispatch here? - Benjamin Bannier On March 15, 2019, 6:15 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70214/ > ----------------------------------------------------------- > > (Updated March 15, 2019, 6:15 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 > ------- > > Added skeleton code for v0 `VolumeManager`. > > > Diffs > ----- > > src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 > src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 > src/csi/v0_volume_manager.hpp PRE-CREATION > src/csi/v0_volume_manager.cpp PRE-CREATION > src/csi/v0_volume_manager_process.hpp PRE-CREATION > src/csi/volume_manager.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70214/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
