----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70215/#review213808 -----------------------------------------------------------
I think it would make sense to add a patch before this one which adds an largely unimplemented `VolumeManager` to SLRP so this patch can start moving first pieces of functionality over (as opposed to a single, big-bang patch https://reviews.apache.org/r/70222/). Should we also add tests for the v0 volume manager? src/csi/v0_volume_manager.cpp Line 120 (original), 142-144 (patched) <https://reviews.apache.org/r/70215/#comment299857> Shouldn't we evaluate this when we actually send the operation in `_attachVolume`? I am worrying about the potential of interleaved, multistep transitions. Here and in all other handlers. src/csi/v0_volume_manager.cpp Lines 146 (patched) <https://reviews.apache.org/r/70215/#comment299859> `s/sequentialized/serialized/` here and below? _attaching_ to be consistent with other methods? src/csi/v0_volume_manager.cpp Lines 159 (patched) <https://reviews.apache.org/r/70215/#comment299858> _detaching_? src/csi/v0_volume_manager.cpp Lines 172 (patched) <https://reviews.apache.org/r/70215/#comment299856> I don't think _publishment_ is the word we are looking for, maybe _publishing_ instead? Here, below and in the commit message. src/csi/v0_volume_manager.cpp Lines 185 (patched) <https://reviews.apache.org/r/70215/#comment299860> _unpublishing_? src/csi/v0_volume_manager.cpp Lines 192 (patched) <https://reviews.apache.org/r/70215/#comment299861> Let's use uppercase initials for even for non-type template parameters, `s/rpc/Rpc/g`. Here and below. src/csi/v0_volume_manager.cpp Lines 202 (patched) <https://reviews.apache.org/r/70215/#comment299862> Break line before `.then`. src/csi/v0_volume_manager.cpp Lines 251 (patched) <https://reviews.apache.org/r/70215/#comment299863> `Future<T>` can be constructed from a `Try<T, E>`, ``` return result; ``` src/csi/v0_volume_manager.cpp Lines 281 (patched) <https://reviews.apache.org/r/70215/#comment299864> Ditto. src/csi/v0_volume_manager.cpp Lines 302-306 (patched) <https://reviews.apache.org/r/70215/#comment299868> This doesn't look like an internal invariant, so could we maybe return a `Failure` here? That way we can in the future simplify this manager so that it can e.g., bring a volume from any state to a target state or fail if impossible (we already do have special handling for some cases). Here and elsewhere. src/csi/v0_volume_manager.cpp Lines 311 (patched) <https://reviews.apache.org/r/70215/#comment299866> Should we also log something similar to l.328 here? Here and elsewhere where functions perform similar early returns. src/csi/v0_volume_manager.cpp Lines 343 (patched) <https://reviews.apache.org/r/70215/#comment299867> Newline here, remove one above? - Benjamin Bannier On March 16, 2019, 12:38 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70215/ > ----------------------------------------------------------- > > (Updated March 16, 2019, 12:38 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 > ------- > > The attachment and publishment 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 sequentialized through the volume's own > sequence in the public-facing functions to avoid racing operations on > the same volume. > > > Diffs > ----- > > src/csi/v0_volume_manager.cpp PRE-CREATION > src/csi/v0_volume_manager_process.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70215/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
