You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/09/08 22:52:40 UTC
Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
Bugs: MESOS-7877
https://issues.apache.org/jira/browse/MESOS-7877
Repository: mesos
Description
-------
These new overloads make it possible to specify a framework ID and to
pass resources as `Resources` instead of as a string.
Diffs
-----
src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
Diff: https://reviews.apache.org/r/62197/diff/1/
Testing
-------
`make check` still passes on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/#review185304
-----------------------------------------------------------
src/tests/mesos.hpp
Line 590 (original), 590 (patched)
<https://reviews.apache.org/r/62197/#comment261610>
Pass by const ref instead. I'll fix this while committing.
- Greg Mann
On Sept. 12, 2017, 11:49 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62197/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2017, 11:49 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
>
>
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These new overloads make it possible to specify a framework ID, executor
> resources as a proto, and the executor ID also as a proto.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
>
>
> Diff: https://reviews.apache.org/r/62197/diff/6/
>
>
> Testing
> -------
>
> `make check` still passes on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/
-----------------------------------------------------------
(Updated Sept. 12, 2017, 11:49 p.m.)
Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
Changes
-------
Cleaned up parameters.
Bugs: MESOS-7877
https://issues.apache.org/jira/browse/MESOS-7877
Repository: mesos
Description
-------
These new overloads make it possible to specify a framework ID, executor
resources as a proto, and the executor ID also as a proto.
Diffs (updated)
-----
src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
Diff: https://reviews.apache.org/r/62197/diff/6/
Changes: https://reviews.apache.org/r/62197/diff/5-6/
Testing
-------
`make check` still passes on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/#review185243
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/mesos.hpp
Lines 590 (patched)
<https://reviews.apache.org/r/62197/#comment261545>
I think that this parameter can be a `const std::string&` instead of an `Option`, which would allow us to get rid of the conditional in the function body.
- Greg Mann
On Sept. 12, 2017, 7:47 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62197/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2017, 7:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
>
>
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These new overloads make it possible to specify a framework ID, executor
> resources as a proto, and the executor ID also as a proto.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
>
>
> Diff: https://reviews.apache.org/r/62197/diff/5/
>
>
> Testing
> -------
>
> `make check` still passes on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/
-----------------------------------------------------------
(Updated Sept. 12, 2017, 7:47 p.m.)
Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
Changes
-------
Removed unused methods + used `createCommandInfo` helper.
Bugs: MESOS-7877
https://issues.apache.org/jira/browse/MESOS-7877
Repository: mesos
Description
-------
These new overloads make it possible to specify a framework ID, executor
resources as a proto, and the executor ID also as a proto.
Diffs (updated)
-----
src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
Diff: https://reviews.apache.org/r/62197/diff/4/
Changes: https://reviews.apache.org/r/62197/diff/3-4/
Testing
-------
`make check` still passes on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Sept. 12, 2017, 5:21 p.m., Greg Mann wrote:
> > src/tests/mesos.hpp
> > Lines 512-522 (original), 620-638 (patched)
> > <https://reviews.apache.org/r/62197/diff/3/?file=1819180#file1819180line621>
> >
> > I'm not sure why this overload was added originally, since `const char*` is implicitly convertible to `std::string`. I tried removing this overload as well and it compiled OK, so perhaps we can eliminate it?
Good idea, removed it.
> On Sept. 12, 2017, 5:21 p.m., Greg Mann wrote:
> > src/tests/mesos.hpp
> > Lines 641-658 (patched)
> > <https://reviews.apache.org/r/62197/diff/3/?file=1819180#file1819180line642>
> >
> > I suspected that this overload wasn't strictly necessary, so I removed it and indeed compilation succeeded. Did you add this proactively to avoid the need to add it in the future?
> >
> > I think we should only add overloads which are actually used in the current codebase. Could you verify which overloads in this patch are actually used?
I added it for consistency with the other method, but we're removing it, so I'm also removing this one.
The other overloads are being used, even though we could probably remove a few ones if we do a sweeping change to make all call sites pass the command as a `CommandInfo` instead of as a string.
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/#review185216
-----------------------------------------------------------
On Sept. 12, 2017, 7:47 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62197/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2017, 7:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
>
>
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These new overloads make it possible to specify a framework ID, executor
> resources as a proto, and the executor ID also as a proto.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
>
>
> Diff: https://reviews.apache.org/r/62197/diff/5/
>
>
> Testing
> -------
>
> `make check` still passes on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/#review185216
-----------------------------------------------------------
src/tests/mesos.hpp
Lines 512-522 (original), 620-638 (patched)
<https://reviews.apache.org/r/62197/#comment261505>
I'm not sure why this overload was added originally, since `const char*` is implicitly convertible to `std::string`. I tried removing this overload as well and it compiled OK, so perhaps we can eliminate it?
src/tests/mesos.hpp
Lines 641-658 (patched)
<https://reviews.apache.org/r/62197/#comment261501>
I suspected that this overload wasn't strictly necessary, so I removed it and indeed compilation succeeded. Did you add this proactively to avoid the need to add it in the future?
I think we should only add overloads which are actually used in the current codebase. Could you verify which overloads in this patch are actually used?
- Greg Mann
On Sept. 9, 2017, 6:52 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62197/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2017, 6:52 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
>
>
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These new overloads make it possible to specify a framework ID, executor
> resources as a proto, and the executor ID also as a proto.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
>
>
> Diff: https://reviews.apache.org/r/62197/diff/3/
>
>
> Testing
> -------
>
> `make check` still passes on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/
-----------------------------------------------------------
(Updated Sept. 9, 2017, 6:52 a.m.)
Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
Changes
-------
Fixed infinite recursion.
Bugs: MESOS-7877
https://issues.apache.org/jira/browse/MESOS-7877
Repository: mesos
Description
-------
These new overloads make it possible to specify a framework ID, executor
resources as a proto, and the executor ID also as a proto.
Diffs (updated)
-----
src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
Diff: https://reviews.apache.org/r/62197/diff/3/
Changes: https://reviews.apache.org/r/62197/diff/2-3/
Testing
-------
`make check` still passes on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/
-----------------------------------------------------------
(Updated Sept. 9, 2017, 12:03 a.m.)
Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
Changes
-------
Added more overloads.
Bugs: MESOS-7877
https://issues.apache.org/jira/browse/MESOS-7877
Repository: mesos
Description (updated)
-------
These new overloads make it possible to specify a framework ID, executor
resources as a proto, and the executor ID also as a proto.
Diffs (updated)
-----
src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
Diff: https://reviews.apache.org/r/62197/diff/2/
Changes: https://reviews.apache.org/r/62197/diff/1-2/
Testing
-------
`make check` still passes on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 62197: Added new overloads for the
`createExecutorInfo` test helper method.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62197/#review185036
-----------------------------------------------------------
ERROR: Failed to apply patch 62197. Please check http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62197/logs/apply-reviews.log for any relevant errors
Reviews applied: [62197]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62197/logs
- Mesos Reviewbot Windows
On Sept. 8, 2017, 10:52 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62197/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 10:52 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad.
>
>
> Bugs: MESOS-7877
> https://issues.apache.org/jira/browse/MESOS-7877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These new overloads make it possible to specify a framework ID and to
> pass resources as `Resources` instead of as a string.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554
>
>
> Diff: https://reviews.apache.org/r/62197/diff/1/
>
>
> Testing
> -------
>
> `make check` still passes on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>