Re: Review Request 62197: Added new overloads for the `createExecutorInfo` test helper method.

[Greg Mann]

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


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.

[Gastón Kleiman]

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

[Greg Mann]

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


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.

[Gastón Kleiman]

> On Sept. 12, 2017, 5:21 p.m., Greg Mann wrote:
> > src/tests/mesos.hpp
> > Lines 512-522 (original), 620-638 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

[Gastón Kleiman]

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

[Greg Mann]

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


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)


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.

[Gastón Kleiman]

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

[Gastón Kleiman]

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

[Mesos Reviewbot Windows]

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