You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/04/08 14:39:23 UTC
Review Request 45926: Cleaned up flag descriptions in mesos-execute.
-----------------------------------------------------------
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
Re: Review Request 45926: Cleaned up flag descriptions in
mesos-execute.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On April 8, 2016, 11:10 a.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 97-100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line97>
> >
> > 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/44441/) 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 r44441 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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 8, 2016, 6:10 p.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 97-100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line97>
> >
> > 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/44441/) 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.
Posted by Guangya Liu <gy...@gmail.com>.
> On April 8, 2016, 6:10 p.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 97-100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line97>
> >
> > 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/44441/) 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 r44441 can replace the TODO.)
Alex, can you pleas leave this command to https://reviews.apache.org/r/44441/ ? 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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/45926/#comment191205>
Not yours, but s/IP1:PORT1/IP:PORT/
src/cli/execute.cpp (lines 97 - 100)
<https://reviews.apache.org/r/45926/#comment191207>
Might be nice to note that this is ignored if `flags.shell` is false.
src/cli/execute.cpp (line 99)
<https://reviews.apache.org/r/45926/#comment191206>
s/shell command, an/shell command, or an/
src/cli/execute.cpp (line 108)
<https://reviews.apache.org/r/45926/#comment191208>
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
>
>
Re: Review Request 45926: Cleaned up flag descriptions in
mesos-execute.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Guangya Liu <gy...@gmail.com>.
> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line100>
> >
> > 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/44441/
In my understanding, what I did in r44441 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.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line100>
> >
> > 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/44441/
>
> Guangya Liu wrote:
> In my understanding, what I did in r44441 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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line100>
> >
> > 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/44441/
>
> Guangya Liu wrote:
> In my understanding, what I did in r44441 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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line100>
> >
> > 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/44441/
- 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.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45926/#review127945
-----------------------------------------------------------
src/cli/execute.cpp (line 100)
<https://reviews.apache.org/r/45926/#comment191336>
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
>
>