> On Aug. 4, 2020, 10:39 p.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Lines 17-22 (patched) > > <https://reviews.apache.org/r/72690/diff/6/?file=2236973#file2236973line17> > > > > Missing includes for types used in this file like `string`, `Future`, > > etc. > > Qian Zhang wrote: > Do we need to do this if we include them in the header file > `csi/isolator.hpp`?
IMHO it's always appropriate to have includes in a file for any types referenced in that file. > On Aug. 4, 2020, 10:39 p.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Lines 41 (patched) > > <https://reviews.apache.org/r/72690/diff/6/?file=2236973#file2236973line41> > > > > Should we do a check for `if (csiServer == nullptr)` in the body of > > `create()`? > > Qian Zhang wrote: > I think we do not need to do that. Like in > `EnvironmentSecretIsolatorProcess::create` and > `VolumeSecretIsolatorProcess::create` we just use the pointer > `secretResolver` but not check if it is a null pointer. In the other two cases you mentioned, it is valid to have `secretResolver == nullptr`. Are there any valid use cases which will have `csiServer == nullptr`? - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72690/#review221468 ----------------------------------------------------------- On Aug. 5, 2020, 8:13 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72690/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2020, 8:13 a.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10152 > https://issues.apache.org/jira/browse/MESOS-10152 > > > Repository: mesos > > > Description > ------- > > Implemented the framework and `create` method of `volume/csi` isolator. > > > Diffs > ----- > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp > PRE-CREATION > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > PRE-CREATION > src/slave/containerizer/mesos/isolators/volume/csi/paths.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/volume/csi/paths.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72690/diff/8/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
