----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46498/#review138884 -----------------------------------------------------------
Please address my comments and followings: 1. Could you add the JIRA ticket MESOS-4778 to `BUG`. That would create a link from review board to JIRA ticket automatically. 2. Could you mention what test you have done on `test done` section? 3. Please make sure to mannually test with a real appc image, and verify the default executable(s) run correctly. 4. Could you move the test case to a separate patch (appreciated that would make review easier). include/mesos/appc/spec.proto (lines 59 - 60) <https://reviews.apache.org/r/46498/#comment204081> Please rephase this comment depending on the spec: https://github.com/appc/spec/blob/master/spec/aci.md#image-manifest-schema include/mesos/appc/spec.proto (line 61) <https://reviews.apache.org/r/46498/#comment204080> Fix the indentation. include/mesos/appc/spec.proto (lines 62 - 64) <https://reviews.apache.org/r/46498/#comment204082> Did you test your patch with a real appc image runtime? It seems to me the `Exec` will not be parsed properly. You may not capture all executable strings. include/mesos/appc/spec.proto (line 66) <https://reviews.apache.org/r/46498/#comment204083> Please see other `TODO` for correct formatting. Thanks:) include/mesos/appc/spec.proto (lines 66 - 75) <https://reviews.apache.org/r/46498/#comment204086> Could we move this above annotation and dependency? Let's make it same order as it spec. More clear for people to read it. :) include/mesos/appc/spec.proto (line 70) <https://reviews.apache.org/r/46498/#comment204084> Should this be repeated string? include/mesos/appc/spec.proto (line 72) <https://reviews.apache.org/r/46498/#comment204092> hmm..seems like we can reuse the `name`-`value` pair from the label. I would suggest introduce another protobuf message `Environment`. include/mesos/slave/isolator.proto (lines 87 - 92) <https://reviews.apache.org/r/46498/#comment204093> Please move it below (at TODO position). include/mesos/slave/isolator.proto (line 91) <https://reviews.apache.org/r/46498/#comment204094> Let's use 10 for proto field #. #6 may be used before. This may make it backward incompatible. - Gilbert Song On June 2, 2016, 8:45 p.m., Srinivas Brahmaroutu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46498/ > ----------------------------------------------------------- > > (Updated June 2, 2016, 8:45 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese. > > > Repository: mesos > > > Description > ------- > > Add runtime for Appc Spec ex: command, workingdir and environment. > > > Diffs > ----- > > include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 > include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 > src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc > src/slave/containerizer/mesos/containerizer.cpp > c7b9744463cf8e1921dcb5e2b7dec7d4e2c0e45f > src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION > src/slave/containerizer/mesos/provisioner/appc/store.cpp > aaa0efe63e587b9e604082b52a3cb8c11545fbb9 > src/slave/containerizer/mesos/provisioner/provisioner.hpp > 48a05059969e068a0ee0d38b61be9e7104e3188d > src/slave/containerizer/mesos/provisioner/provisioner.cpp > 7540be6d8a412eb3d380d315c59223236d3eff67 > src/slave/containerizer/mesos/provisioner/store.hpp > 1d477ef13ddd24fd8badae0decaa4a2271ecc746 > src/tests/containerizer/provisioner_appc_tests.cpp > 84fe52b6937c3b7d7628b17a2f045eec2f386b4d > > Diff: https://reviews.apache.org/r/46498/diff/ > > > Testing > ------- > > > Thanks, > > Srinivas Brahmaroutu > >
