----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65900/#review203611 -----------------------------------------------------------
This commit contains both refactoring and functional changes. We should split the refactoring into a separate patch so that they can be reviewed and tested independently. The commit message for that should explain the context of why the refactoring is necessary. Once that is done, then we can make the functional changes in this patch, but I think the ordering is wrong. This patch removes the code that creates the mount points, and that functionality is not restored until later in the series. We need to add all the necessary support earlier in the series, then switch the callers over to it in a single patch so that there is never a commit where things are not working. Once you tackle the above points, I think that the commit message for this patch needs a lot more detail. We need to explain the context of this change and give the reader information about why this change is important (ie. what is the benefit of making the change) and some more concrete information about what is being changed. src/slave/containerizer/mesos/isolators/volume/utils.hpp Lines 18 (patched) <https://reviews.apache.org/r/65900/#comment285900> There's no need for this to be header-only, let's make a .cpp and a .hpp. src/slave/containerizer/mesos/isolators/volume/utils.hpp Lines 40 (patched) <https://reviews.apache.org/r/65900/#comment285901> This really boils down to separate code paths depending on whether there is a `rootfs`. Can we break this into 2 separate functions, mirroring the original code? That's easier to understand, since we don't have a function that is performing double-duty. - James Peach On May 17, 2018, 1:16 a.m., Jason Lai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65900/ > ----------------------------------------------------------- > > (Updated May 17, 2018, 1:16 a.m.) > > > Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, > James Peach, and Zhitao Li. > > > Bugs: MESOS-8257 > https://issues.apache.org/jira/browse/MESOS-8257 > > > Repository: mesos > > > Description > ------- > > Defer creation of volume target paths to container launch. > > > Diffs > ----- > > src/Makefile.am c08ac6e2f5deec4d05f59f71ff6c51382f216708 > src/slave/containerizer/mesos/isolators/volume/host_path.cpp > 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a > src/slave/containerizer/mesos/isolators/volume/image.cpp > 631553e2f61a1b95dd93d333b547ff237f65f59e > src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp > e0cae1036e2e49b4f61705c77f31ae166d1b1380 > src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/65900/diff/3/ > > > Testing > ------- > > > Thanks, > > Jason Lai > >
