Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-06-21 Thread Gilbert Song 

---
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)


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)


Fix the indentation.



include/mesos/appc/spec.proto (lines 62 - 64)


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)


Please see other `TODO` for correct formatting. Thanks:)



include/mesos/appc/spec.proto (lines 66 - 75)


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)


Should this be repeated string?



include/mesos/appc/spec.proto (line 72)


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)


Please move it below (at TODO position).



include/mesos/slave/isolator.proto (line 91)


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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-06-02 Thread Srinivas Brahmaroutu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/
---

(Updated June 3, 2016, 3:45 a.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 (updated)
-

  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

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-20 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review134229
---



Patch looks great!

Reviews applied: [46498]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 21, 2016, 12:24 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 21, 2016, 12:24 a.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 b1c84aeb27fb86330f501ed389ec1d696cdf11a9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-20 Thread Srinivas Brahmaroutu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/
---

(Updated May 21, 2016, 12:24 a.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 (updated)
-

  include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
  include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
  src/Makefile.am b1c84aeb27fb86330f501ed389ec1d696cdf11a9 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  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 
33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
5c96e9f6603d39889e6bc807874d35d0cb3556be 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
  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

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-20 Thread Srinivas Brahmaroutu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/
---

(Updated May 20, 2016, 10:59 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 (updated)
-

  include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
  include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
  src/Makefile.am b1c84aeb27fb86330f501ed389ec1d696cdf11a9 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  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 
33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
5c96e9f6603d39889e6bc807874d35d0cb3556be 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
  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

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-16 Thread Guangya Liu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133483
---




include/mesos/appc/spec.proto (line 58)


Better add some comments here for Exec



src/slave/containerizer/mesos/containerizer.cpp (line 79)


This should be 
#ifdef __linux__
#include "slave/containerizer/mesos/isolators/appc/runtime.hpp"
#endif


- Guangya Liu


On May 14, 2016, 4:20 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 14, 2016, 4:20 a.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 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-16 Thread Jojy Varghese 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133390
---




src/slave/containerizer/mesos/containerizer.cpp (line 953)


It would be readable if we separat the two `if` blocks with a blanlk line.



src/slave/containerizer/mesos/isolators/appc/runtime.hpp (line 26)


s/APPC/AppC? Please look for more occurrences.



src/slave/containerizer/mesos/isolators/appc/runtime.hpp (line 29)


s/Appc/AppC as Guangya proposed?



src/slave/containerizer/mesos/isolators/appc/runtime.hpp (lines 64 - 73)


Do these need to be member functions? Could they be functions in `cpp` file?



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 50)


No need to wrap to newline. It fits within 80 chars.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 147)


blank line above.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 171)


blank line above.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 174)


blank line above



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 246)


I think you should use `string::empty`



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 201)


Do you need this variable?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 203)


I think you just need the base image manifest here and not all the 
manifests. Right?


- Jojy Varghese


On May 14, 2016, 4:20 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 14, 2016, 4:20 a.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 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-14 Thread Guangya Liu 


> On 五月 12, 2016, 11:44 p.m., Jojy Varghese wrote:
> > Srinivas, thanks for taking this on. It looks like you combined your 
> > previous patches (https://reviews.apache.org/r/46107) here. I would advice 
> > you to split this patch into 2:
> >  - Protobuf changes
> >  - Other changes
> >  
> > 
> > -jojy
> 
> Srinivas Brahmaroutu wrote:
> Hi Jojy, I would rebase as the other two patches get merged. This patch 
> includes other two patches https://reviews.apache.org/r/46107 and 
> https://reviews.apache.org/r/46182  I have cumlative patches. I may be wrong, 
> but once https://reviews.apache.org/r/46107 is checked in I will rebase 46182 
> and so on?

Srini, you can create a patch chain that have dependeny to resolve this, the 
steps could be as this:
1) git checkout -b part1
2) Update your code and `support/post-review.sh`
3) git checkout -b part2 (This can make sure part2 depend on part1)
4) Update your code and `support/post-review.sh`
5) repeat 3) and 4) for other patches


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133004
---


On 五月 14, 2016, 4:20 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated 五月 14, 2016, 4:20 a.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 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-14 Thread Guangya Liu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133256
---



Some early comments, can you please split this to small patches? You can refer 
to https://reviews.apache.org/r/43021/ patch chain for how does run time 
isolator did this.

I saw that you also have patches https://reviews.apache.org/r/46107 and 
https://reviews.apache.org/r/46182/ , seems they are duplicate with this?


include/mesos/appc/spec.proto (line 58)


blank line above



include/mesos/appc/spec.proto (lines 67 - 68)


Why 6, 7? I think should be 2, 3



include/mesos/appc/spec.proto (line 68)


s/environment/environments



include/mesos/slave/isolator.proto (line 21)


blank line below



include/mesos/slave/isolator.proto (lines 86 - 91)


s/Appc/AppC

Why not `optional appc.spec.ImageManifest manifest = 6;`

Or 

message Appc {
  required appc.spec.ImageManifest manifest = 1;
}

optional Appc appc = 6;

I think that the field in `AppC` should be `required`



src/Makefile.am (line 948)


align with others



src/Makefile.am (line 971)


align with others



src/slave/containerizer/mesos/containerizer.cpp (line 79)


I prefer that you put it into a new #ifdef as it is a new isolator.

#ifdef __linux__
#include "slave/containerizer/mesos/isolators/mesos/runtime.hpp"
#endif



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 27)


kill this



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 199)


kill this



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 209 - 212)


I think that we still have `argv`, why you are ignoring `argv` here? It is 
better use a table same as `DockerRuntimeIsolatorProcess` to describe the 
behavior here.



src/slave/containerizer/mesos/provisioner/provisioner.hpp (lines 24 - 25)


#include 

#include 


- Guangya Liu


On 五月 14, 2016, 4:20 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated 五月 14, 2016, 4:20 a.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 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-13 Thread Srinivas Brahmaroutu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/
---

(Updated May 14, 2016, 4:20 a.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 (updated)
-

  include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
  include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
  src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  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 
33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
5c96e9f6603d39889e6bc807874d35d0cb3556be 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
  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

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-13 Thread Srinivas Brahmaroutu 


> On May 12, 2016, 11:44 p.m., Jojy Varghese wrote:
> > Srinivas, thanks for taking this on. It looks like you combined your 
> > previous patches (https://reviews.apache.org/r/46107) here. I would advice 
> > you to split this patch into 2:
> >  - Protobuf changes
> >  - Other changes
> >  
> > 
> > -jojy

Hi Jojy, I would rebase as the other two patches get merged. This patch 
includes other two patches https://reviews.apache.org/r/46107 and 
https://reviews.apache.org/r/46182  I have cumlative patches. I may be wrong, 
but once https://reviews.apache.org/r/46107 is checked in I will rebase 46182 
and so on?


- Srinivas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133004
---


On May 12, 2016, 9:23 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 12, 2016, 9:23 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 922b5984b67ace97c142b52aaa8221323147a7ff 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1e1a36903f4377497bb72b69e4ead63675d453c0 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-12 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133014
---



Bad patch!

Reviews applied: [46498]

Failed command: ./support/apply-review.sh -n -r 46498

Error:
2016-05-13 01:39:51 URL:https://reviews.apache.org/r/46498/diff/raw/ 
[25311/25311] -> "46498.patch" [1]
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:246
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13034/console

- Mesos ReviewBot


On May 12, 2016, 9:23 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 12, 2016, 9:23 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 922b5984b67ace97c142b52aaa8221323147a7ff 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1e1a36903f4377497bb72b69e4ead63675d453c0 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>

Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-12 Thread Jojy Varghese 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46498/#review133004
---



Srinivas, thanks for taking this on. It looks like you combined your previous 
patches (https://reviews.apache.org/r/46107) here. I would advice you to split 
this patch into 2:
 - Protobuf changes
 - Other changes
 

-jojy

- Jojy Varghese


On May 12, 2016, 9:23 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 12, 2016, 9:23 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 922b5984b67ace97c142b52aaa8221323147a7ff 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1e1a36903f4377497bb72b69e4ead63675d453c0 
>   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 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   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
> 
>