You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Timothy Chen <tn...@apache.org> on 2014/08/15 23:49:49 UTC
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 9:49 p.m.)
Review request for drill, Benjamin Hindman and Jie Yu.
Summary (updated)
-----------------
Allow override without CommandInfo value to run in command executor
Repository: mesos-git
Description (updated)
-------
Review: https://reviews.apache.org/r/24730
Diffs (updated)
-----
src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
Diff: https://reviews.apache.org/r/24730/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/
-----------------------------------------------------------
(Updated Aug. 16, 2014, 12:07 a.m.)
Review request for mesos, Benjamin Hindman and Jie Yu.
Repository: mesos-git
Description
-------
Allow override without CommandInfo value to run in command executor
Diffs (updated)
-----
src/launcher/executor.cpp 1aa2c99
src/tests/docker_containerizer_tests.cpp e0fd62f
Diff: https://reviews.apache.org/r/24730/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50790
-----------------------------------------------------------
Ship it!
src/launcher/executor.cpp
<https://reviews.apache.org/r/24730/#comment88656>
Add a print to indicate it is an override?
```c++
command = "override: "
// argv is guaranteed to be NULL terminated and we rely on
// that fact to print command to be executed.
char** argv = override.get();
for (int i = 0; argv[i] != NULL; i++) {
command += ...;
}
```
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/24730/#comment88657>
Can you expand a little bit here? Why setting value (not arguments)?
- Jie Yu
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Aug. 15, 2014, 11:14 p.m., Timothy Chen wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1108-1110
> > <https://reviews.apache.org/r/24730/diff/2/?file=661859#file661859line1108>
> >
> > It's really just to test out value field explicitly. There are a lot more variations but just trying to touch on the common ones.
>
> Jie Yu wrote:
> OK, I guess I missed this part in last patch. So if shell == false, 'value' will be appended to the docker run according to the code. I was expecting that 'value' will be ignored and only appending arguments. Maybe you guys should add some comments to call out this semantics explicitly.
>
> Timothy Chen wrote:
> We plan to add more documentation around DockerInfo once all our work are in there :)
Yes, definitely Jie! I added more comments to the test, will update the diff.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50791
-----------------------------------------------------------
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Timothy Chen <tn...@apache.org>.
> On Aug. 15, 2014, 11:14 p.m., Timothy Chen wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1108-1110
> > <https://reviews.apache.org/r/24730/diff/2/?file=661859#file661859line1108>
> >
> > It's really just to test out value field explicitly. There are a lot more variations but just trying to touch on the common ones.
>
> Jie Yu wrote:
> OK, I guess I missed this part in last patch. So if shell == false, 'value' will be appended to the docker run according to the code. I was expecting that 'value' will be ignored and only appending arguments. Maybe you guys should add some comments to call out this semantics explicitly.
We plan to add more documentation around DockerInfo once all our work are in there :)
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50791
-----------------------------------------------------------
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 15, 2014, 11:14 p.m., Timothy Chen wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1108-1110
> > <https://reviews.apache.org/r/24730/diff/2/?file=661859#file661859line1108>
> >
> > It's really just to test out value field explicitly. There are a lot more variations but just trying to touch on the common ones.
OK, I guess I missed this part in last patch. So if shell == false, 'value' will be appended to the docker run according to the code. I was expecting that 'value' will be ignored and only appending arguments. Maybe you guys should add some comments to call out this semantics explicitly.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50791
-----------------------------------------------------------
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Timothy Chen <tn...@apache.org>.
> On Aug. 15, 2014, 11:14 p.m., Timothy Chen wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1108-1110
> > <https://reviews.apache.org/r/24730/diff/2/?file=661859#file661859line1108>
> >
> > It's really just to test out value field explicitly. There are a lot more variations but just trying to touch on the common ones.
>
> Jie Yu wrote:
> OK, I guess I missed this part in last patch. So if shell == false, 'value' will be appended to the docker run according to the code. I was expecting that 'value' will be ignored and only appending arguments. Maybe you guys should add some comments to call out this semantics explicitly.
>
> Timothy Chen wrote:
> We plan to add more documentation around DockerInfo once all our work are in there :)
>
> Benjamin Hindman wrote:
> Yes, definitely Jie! I added more comments to the test, will update the diff.
>
> Benjamin Hindman wrote:
> Oh wait, this isn't my review! ;-) But I did update Tim's review and added more commments.
:) How do I get these updates then?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50791
-----------------------------------------------------------
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Aug. 15, 2014, 11:14 p.m., Timothy Chen wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1108-1110
> > <https://reviews.apache.org/r/24730/diff/2/?file=661859#file661859line1108>
> >
> > It's really just to test out value field explicitly. There are a lot more variations but just trying to touch on the common ones.
>
> Jie Yu wrote:
> OK, I guess I missed this part in last patch. So if shell == false, 'value' will be appended to the docker run according to the code. I was expecting that 'value' will be ignored and only appending arguments. Maybe you guys should add some comments to call out this semantics explicitly.
>
> Timothy Chen wrote:
> We plan to add more documentation around DockerInfo once all our work are in there :)
>
> Benjamin Hindman wrote:
> Yes, definitely Jie! I added more comments to the test, will update the diff.
Oh wait, this isn't my review! ;-) But I did update Tim's review and added more commments.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50791
-----------------------------------------------------------
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50791
-----------------------------------------------------------
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/24730/#comment88658>
It's really just to test out value field explicitly. There are a lot more variations but just trying to touch on the common ones.
- Timothy Chen
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/#review50788
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Aug. 15, 2014, 9:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24730/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 9:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Allow override without CommandInfo value to run in command executor
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24730/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 24730: Allow override without CommandInfo value to
run in command executor
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24730/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 9:50 p.m.)
Review request for mesos, Benjamin Hindman and Jie Yu.
Repository: mesos-git
Description (updated)
-------
Allow override without CommandInfo value to run in command executor
Diffs
-----
src/launcher/executor.cpp 1aa2c99a5e5b19982e99f03d6fbb3bc3d34cea64
src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
Diff: https://reviews.apache.org/r/24730/diff/
Testing
-------
make check
Thanks,
Timothy Chen