Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-12 Thread Guangya Liu 


> On 四月 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > 
> >
> > I think we cannot sepcify the entrypoint for container here, the 
> > entrypoint was runtime configuration and was get from docker iamge.
> 
> Alexander Rukletsov wrote:
> Why not? That's exactly what you do in https://reviews.apache.org/r/1/
> 
> Guangya Liu wrote:
> In my understanding, what I did in r1 is for 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L248-L256
>  , but the entrypoint is 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L240-L248
>  , make sense?
> 
> Alexander Rukletsov wrote:
> No, it doesn't. You have branching in your patch `if (command.isSome()) 
> {` which switches between the two cases and hence covers them both.

Alex, you may see that the logic 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L273-L284
 here is handlign the command and the entry point logic handling is under line 
284. I think that we cannot use command to specify an entrypoint for 
mesos-execute, if we can, how can I specify the command in mesos execute? 
Thanks.


- Guangya


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


On 四月 12, 2016, 9:58 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated 四月 12, 2016, 9:58 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-12 Thread Alexander Rukletsov 

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

(Updated April 12, 2016, 9:58 a.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 

Diff: https://reviews.apache.org/r/45926/diff/


Testing
---

See the last patch in the chain: https://reviews.apache.org/r/45927/


Thanks,

Alexander Rukletsov

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-12 Thread Alexander Rukletsov 


> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > 
> >
> > I think we cannot sepcify the entrypoint for container here, the 
> > entrypoint was runtime configuration and was get from docker iamge.
> 
> Alexander Rukletsov wrote:
> Why not? That's exactly what you do in https://reviews.apache.org/r/1/
> 
> Guangya Liu wrote:
> In my understanding, what I did in r1 is for 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L248-L256
>  , but the entrypoint is 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L240-L248
>  , make sense?

No, it doesn't. You have branching in your patch `if (command.isSome()) {` 
which switches between the two cases and hence covers them both.


- Alexander


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


On April 11, 2016, 7 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 11, 2016, 7 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-11 Thread Guangya Liu 


> On April 8, 2016, 6:10 p.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 97-100
> > 
> >
> > Might be nice to note that this is ignored if `flags.shell` is false.
> 
> Alexander Rukletsov wrote:
> It's true for the _current_ code in mesos-execute, but not true for 
> Mesos. Moreover, there is a patch in the works 
> (https://reviews.apache.org/r/1/) which adds support for this.
> 
> Joseph Wu wrote:
> These flags are only used by `mesos-execute` right?  You could just 
> surface the TODO in the code into the flag.  (And r1 can replace the 
> TODO.)

Alex, can you pleas leave this command to https://reviews.apache.org/r/1/ ? 
I was also trying to update this flag.


- Guangya


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


On April 11, 2016, 7 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 11, 2016, 7 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-11 Thread Joseph Wu 


> On April 8, 2016, 11:10 a.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 97-100
> > 
> >
> > Might be nice to note that this is ignored if `flags.shell` is false.
> 
> Alexander Rukletsov wrote:
> It's true for the _current_ code in mesos-execute, but not true for 
> Mesos. Moreover, there is a patch in the works 
> (https://reviews.apache.org/r/1/) which adds support for this.

These flags are only used by `mesos-execute` right?  You could just surface the 
TODO in the code into the flag.  (And r1 can replace the TODO.)


- Joseph


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


On April 11, 2016, noon, Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 11, 2016, noon)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-11 Thread Guangya Liu 


> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > 
> >
> > I think we cannot sepcify the entrypoint for container here, the 
> > entrypoint was runtime configuration and was get from docker iamge.
> 
> Alexander Rukletsov wrote:
> Why not? That's exactly what you do in https://reviews.apache.org/r/1/

In my understanding, what I did in r1 is for 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L248-L256
 , but the entrypoint is 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L240-L248
 , make sense?


- Guangya


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


On April 11, 2016, 7 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 11, 2016, 7 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-11 Thread Alexander Rukletsov 

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

(Updated April 11, 2016, 7 p.m.)


Review request for mesos, Anand Mazumdar and Joseph Wu.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 

Diff: https://reviews.apache.org/r/45926/diff/


Testing
---

See the last patch in the chain: https://reviews.apache.org/r/45927/


Thanks,

Alexander Rukletsov

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-11 Thread Alexander Rukletsov 


> On April 8, 2016, 6:10 p.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 97-100
> > 
> >
> > Might be nice to note that this is ignored if `flags.shell` is false.

It's true for the _current_ code in mesos-execute, but not true for Mesos. 
Moreover, there is a patch in the works (https://reviews.apache.org/r/1/) 
which adds support for this.


- Alexander


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


On April 8, 2016, 12:39 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 8, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-11 Thread Alexander Rukletsov 


> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > 
> >
> > I think we cannot sepcify the entrypoint for container here, the 
> > entrypoint was runtime configuration and was get from docker iamge.

Why not? That's exactly what you do in https://reviews.apache.org/r/1/


- Alexander


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


On April 8, 2016, 12:39 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 8, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-08 Thread Guangya Liu 

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




src/cli/execute.cpp (line 100)


I think we cannot sepcify the entrypoint for container here, the entrypoint 
was runtime configuration and was get from docker iamge.


- Guangya Liu


On 四月 8, 2016, 12:39 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated 四月 8, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-08 Thread Joseph Wu 

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


Fix it, then Ship it!





src/cli/execute.cpp (line 85)


Not yours, but s/IP1:PORT1/IP:PORT/



src/cli/execute.cpp (lines 97 - 100)


Might be nice to note that this is ignored if `flags.shell` is false.



src/cli/execute.cpp (line 99)


s/shell command, an/shell command, or an/



src/cli/execute.cpp (line 108)


This format (`/path/to/file`) is deprecated, so I'd vote for removing it 
from the description.


- Joseph Wu


On April 8, 2016, 5:39 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
> 
> (Updated April 8, 2016, 5:39 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/45926/diff/
> 
> 
> Testing
> ---
> 
> See the last patch in the chain: https://reviews.apache.org/r/45927/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 45926: Cleaned up flag descriptions in mesos-execute.

2016-04-08 Thread Alexander Rukletsov 

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

Review request for mesos, Anand Mazumdar and Joseph Wu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 

Diff: https://reviews.apache.org/r/45926/diff/


Testing
---

See the last patch in the chain: https://reviews.apache.org/r/45927/


Thanks,

Alexander Rukletsov