----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49348/#review140475 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 80) <https://reviews.apache.org/r/49348/#comment205855> As your previous patches are using `Appc` in comments, can you please use `Appc` here and other comments as well? src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 87 - 88) <https://reviews.apache.org/r/49348/#comment205856> One line src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 161) <https://reviews.apache.org/r/49348/#comment205878> Can you please add a comment here just like what we did for docker run time isolator for how to handle duplicate env vars? // Keep all environment from runtime isolator. If there exists // environment variable duplicate cases, it will be overwritten // in mesos containerizer. src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 162) <https://reviews.apache.org/r/49348/#comment205877> kill this line src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 171) <https://reviews.apache.org/r/49348/#comment205879> s/form/from src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 192) <https://reviews.apache.org/r/49348/#comment205880> kill this line src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 202 - 205) <https://reviews.apache.org/r/49348/#comment205882> 1. s/Exec/'Exec' 2. Add period to the end of each comment. It would be great to add more comments to the end for the commnets by refering to the row of the table. Such as: // 1. If 'shell' is false, and 'value' is not set, use Exec from the image. (row 1-2) // 2. If 'shell' is false and 'value' is set, ignore Exec from the image. (row 3-4) // 3. If 'shell' is true and 'value' is not set, return error. (row 5-6) // 4. If 'shell' is true and 'value' is set, ignore Exec from the image. (row 7-8) src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 243) <https://reviews.apache.org/r/49348/#comment205883> I think checking `!command.has_value()` is good enough. The logic here has some problem, if the comand does not have value then the check of `comand.value()` may core dump here. if (!command.has_value()) { return Error("Shell specified but no command value provided"); } Or do you mean `if (!command.has_value() || !command.value().empty()) {` here. src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 257 - 258) <https://reviews.apache.org/r/49348/#comment205884> new line here src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 282) <https://reviews.apache.org/r/49348/#comment205881> s/WorkingDir/workingDirectory - Guangya Liu On 七月 1, 2016, 9:44 p.m., Srinivas Brahmaroutu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49348/ > ----------------------------------------------------------- > > (Updated 七月 1, 2016, 9:44 p.m.) > > > Review request for mesos. > > > Repository: mesos > > > Description > ------- > > Added implementation to Appc Runtime Isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/49348/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Srinivas Brahmaroutu > >
