> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/volume/host_path.hpp > > Lines 23 (patched) > > <https://reviews.apache.org/r/69286/diff/2/?file=2124791#file2124791line23> > > > > nits: > > > > newline above (usually we do that if the hierarchy of the file is > > different) > > > > thanks!
Makes sense. Done. > On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/volume/host_path.hpp > > Lines 47 (patched) > > <https://reviews.apache.org/r/69286/diff/2/?file=2124791#file2124791line47> > > > > Nits: > > > > could you do? > > ``` > > const volume::PathValidator& pathValidator > > ``` Done. > On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/volume/host_path.hpp > > Lines 50 (patched) > > <https://reviews.apache.org/r/69286/diff/2/?file=2124791#file2124791line50> > > > > ditto Done. > On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/volume/host_path.cpp > > Lines 70-71 (original), 73-78 (patched) > > <https://reviews.apache.org/r/69286/diff/2/?file=2124792#file2124792line73> > > > > nits: > > > > probably I would prefer more explicit: > > ``` > > Owned<MesosIsolatorProcess> process; > > if (flags.host_path_volume_force_creation.isSome()) { > > process = new VolumeHostPathIsolatorProcess( > > flags, > > > > PathValidator::parse(flags.host_path_volume_force_creation.get())); > > } else { > > process = new VolumeHostPathIsolatorProcess(flags); > > } > > ``` `Owned` doesn't support assignment after initialization, so the previous ternary-operator way would make more sense. However, I adjusted my code to the styling you suggested in our private discussion. > On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/volume/host_path.cpp > > Lines 189 (patched) > > <https://reviews.apache.org/r/69286/diff/2/?file=2124792#file2124792line189> > > > > nits: > > > > I would suggest to be more explicit on error msg. > > > > ``` > > return Failure("Failed to normalize the host path '" + hostPath.get() > > + "': " + normalizedPath.error()); > > ``` Sure thing. > On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/volume/utils.cpp > > Lines 41 (patched) > > <https://reviews.apache.org/r/69286/diff/2/?file=2124794#file2124794line41> > > > > instead of returning a boolean, do you think it is better to return a > > `Try<Nothing>` (if we always regard `false` as a error case)? Makes sense. Done. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69286/#review213043 ----------------------------------------------------------- On Feb. 11, 2019, 11:12 p.m., Jason Lai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69286/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2019, 11:12 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, > and James Peach. > > > Bugs: MESOS-9009 > https://issues.apache.org/jira/browse/MESOS-9009 > > > Repository: mesos > > > Description > ------- > > Added a new agent flag `--host_path_volume_force_creation` for > the `volume/host_path` isolator. The flag takes a colon-separated > whitelist of paths, under which non-existing host paths are allowed to > be created. > > If the flag is not specified, the isolator behaves in the original way > of prohibiting all non-existing host paths from being created. > > > Diffs > ----- > > src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 > src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 > src/slave/containerizer/mesos/isolators/volume/host_path.hpp > 4b509e91a056381ca90293d16a400ea4368234a3 > src/slave/containerizer/mesos/isolators/volume/host_path.cpp > 88ecf91d91e2bebd484a4ac94510a14b3500dbfb > src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION > src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 > src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 > > > Diff: https://reviews.apache.org/r/69286/diff/2/ > > > Testing > ------- > > > Thanks, > > Jason Lai > >
