You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Armand Grillet <ag...@mesosphere.io> on 2017/08/07 12:50:28 UTC
Re: Review Request 61111: Extracted strings into constants in example
frameworks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61111/
-----------------------------------------------------------
(Updated Aug. 7, 2017, 12:50 p.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Changes
-------
Applied changes on every example frameworks.
Bugs: MESOS-7814
https://issues.apache.org/jira/browse/MESOS-7814
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
src/examples/balloon_framework.cpp dfd049b860345adc33c3e774dcdff0320da107f6
src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a
src/examples/docker_no_executor_framework.cpp 4a58f11fc8892f23ade1d8c872ab9b4fc580d478
src/examples/dynamic_reservation_framework.cpp f3b1c8c4d2684e827fc10776fef4f2287e315b85
src/examples/load_generator_framework.cpp abb70f42a3a755afaa1d56b1491058b90958f030
src/examples/long_lived_framework.cpp 2a79dfd3f81e5254922895af45c2b72d80ce5f49
src/examples/no_executor_framework.cpp 7d841c6f364e1b671ec829aa9bff3b1f8ecf55ef
src/examples/persistent_volume_framework.cpp 17140294a1eaeeeb92e9e14fc8638182b3a0682e
src/examples/test_framework.cpp 9dbc18b039ee13cc1ec9454bd41cb3cfe30d63b4
src/examples/test_http_framework.cpp a0cd84ab6dfdd8d6ed3403c6e06fabae6d5b46c9
Diff: https://reviews.apache.org/r/61111/diff/5/
Changes: https://reviews.apache.org/r/61111/diff/4-5/
Testing
-------
$ make check
Thanks,
Armand Grillet
Re: Review Request 61111: Extracted strings into constants in example
frameworks.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 8, 2017, 4:35 p.m., Benno Evers wrote:
> > src/examples/balloon_framework.cpp
> > Line 520 (original), 524 (patched)
> > <https://reviews.apache.org/r/61111/diff/6/?file=1790499#file1790499line525>
> >
> > No real issue, but I find it curious that our style guide basically forces us to make redundant string temporaries here, by insisting on `constexpr char[]` over `const string`-literals. Seems like a bug in the style guide?
One main issue when using static `std::string`s would be that `std::string` has a non-trivial destructor which makes it hard to reason about behavior on library unloading or shutdown in general. The Google style guide (on which the Mesos style guide is based) gives some more detail,
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
While we probably do not do enough of a job of preventing calling likely expensive (copy) constructors, I believe that the performance impact in this particular case is small (code is called once in `main`). I also suspect that we already incur a couple orders of magnitude more string (copy) constructors through other temporary objects created.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61111/#review182390
-----------------------------------------------------------
On Aug. 7, 2017, 2:50 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61111/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2017, 2:50 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp dfd049b860345adc33c3e774dcdff0320da107f6
> src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a
> src/examples/docker_no_executor_framework.cpp 4a58f11fc8892f23ade1d8c872ab9b4fc580d478
> src/examples/dynamic_reservation_framework.cpp f3b1c8c4d2684e827fc10776fef4f2287e315b85
> src/examples/load_generator_framework.cpp abb70f42a3a755afaa1d56b1491058b90958f030
> src/examples/long_lived_framework.cpp 2a79dfd3f81e5254922895af45c2b72d80ce5f49
> src/examples/no_executor_framework.cpp 7d841c6f364e1b671ec829aa9bff3b1f8ecf55ef
> src/examples/persistent_volume_framework.cpp 17140294a1eaeeeb92e9e14fc8638182b3a0682e
> src/examples/test_framework.cpp 9dbc18b039ee13cc1ec9454bd41cb3cfe30d63b4
> src/examples/test_http_framework.cpp a0cd84ab6dfdd8d6ed3403c6e06fabae6d5b46c9
>
>
> Diff: https://reviews.apache.org/r/61111/diff/6/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61111: Extracted strings into constants in example
frameworks.
Posted by Benno Evers <be...@yandex-team.ru>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61111/#review182390
-----------------------------------------------------------
Ship it!
src/examples/balloon_framework.cpp
Line 520 (original), 524 (patched)
<https://reviews.apache.org/r/61111/#comment258279>
No real issue, but I find it curious that our style guide basically forces us to make redundant string temporaries here, by insisting on `constexpr char[]` over `const string`-literals. Seems like a bug in the style guide?
- Benno Evers
On Aug. 7, 2017, 12:50 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61111/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2017, 12:50 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp dfd049b860345adc33c3e774dcdff0320da107f6
> src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a
> src/examples/docker_no_executor_framework.cpp 4a58f11fc8892f23ade1d8c872ab9b4fc580d478
> src/examples/dynamic_reservation_framework.cpp f3b1c8c4d2684e827fc10776fef4f2287e315b85
> src/examples/load_generator_framework.cpp abb70f42a3a755afaa1d56b1491058b90958f030
> src/examples/long_lived_framework.cpp 2a79dfd3f81e5254922895af45c2b72d80ce5f49
> src/examples/no_executor_framework.cpp 7d841c6f364e1b671ec829aa9bff3b1f8ecf55ef
> src/examples/persistent_volume_framework.cpp 17140294a1eaeeeb92e9e14fc8638182b3a0682e
> src/examples/test_framework.cpp 9dbc18b039ee13cc1ec9454bd41cb3cfe30d63b4
> src/examples/test_http_framework.cpp a0cd84ab6dfdd8d6ed3403c6e06fabae6d5b46c9
>
>
> Diff: https://reviews.apache.org/r/61111/diff/6/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61111: Extracted strings into constants in example
frameworks.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61111/#review182578
-----------------------------------------------------------
Fix it, then Ship it!
I will fix outstanding issues and commit it for you.
src/examples/disk_full_framework.cpp
Line 59 (original), 60-61 (patched)
<https://reviews.apache.org/r/61111/#comment258527>
Let's keep the original double blank line space.
src/examples/load_generator_framework.cpp
Line 44 (original), 44-45 (patched)
<https://reviews.apache.org/r/61111/#comment258528>
Extra blank line?
src/examples/long_lived_framework.cpp
Lines 100 (patched)
<https://reviews.apache.org/r/61111/#comment258509>
`ExecutorInfo.source` is deprecated since Mesos 1.0. Let's remove it altogether. As a separate commit.
src/examples/long_lived_framework.cpp
Line 98 (original), 102-103 (patched)
<https://reviews.apache.org/r/61111/#comment258529>
Let's keep the original double blank line
src/examples/long_lived_framework.cpp
Line 584 (original), 592 (patched)
<https://reviews.apache.org/r/61111/#comment258508>
This field is deprecated since Mesos 1.0. Let's remove it altogether.
src/examples/no_executor_framework.cpp
Lines 46 (patched)
<https://reviews.apache.org/r/61111/#comment258562>
Missing `;` at the end.
src/examples/persistent_volume_framework.cpp
Line 55 (original), 55-56 (patched)
<https://reviews.apache.org/r/61111/#comment258530>
Ditto
src/examples/test_framework.cpp
Lines 56 (patched)
<https://reviews.apache.org/r/61111/#comment258531>
Ditto
src/examples/test_framework.cpp
Lines 58 (patched)
<https://reviews.apache.org/r/61111/#comment258565>
Missing []?
src/examples/test_framework.cpp
Lines 225-227 (original), 231-233 (patched)
<https://reviews.apache.org/r/61111/#comment258538>
```
uri =
path::join(os::realpath(Path(argv[0]).dirname()).get(), EXECUTOR_BINARY);
```
?
src/examples/test_framework.cpp
Line 255 (original), 261 (patched)
<https://reviews.apache.org/r/61111/#comment258532>
Ditto
src/examples/test_http_framework.cpp
Lines 70 (patched)
<https://reviews.apache.org/r/61111/#comment258533>
Ditto
src/examples/test_http_framework.cpp
Lines 411-413 (original), 416-418 (patched)
<https://reviews.apache.org/r/61111/#comment258540>
Can it fit two lines now?
src/examples/test_http_framework.cpp
Line 459 (original), 464 (patched)
<https://reviews.apache.org/r/61111/#comment258534>
Ditto
- Alexander Rukletsov
On Aug. 7, 2017, 12:50 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61111/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2017, 12:50 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp dfd049b860345adc33c3e774dcdff0320da107f6
> src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a
> src/examples/docker_no_executor_framework.cpp 4a58f11fc8892f23ade1d8c872ab9b4fc580d478
> src/examples/dynamic_reservation_framework.cpp f3b1c8c4d2684e827fc10776fef4f2287e315b85
> src/examples/load_generator_framework.cpp abb70f42a3a755afaa1d56b1491058b90958f030
> src/examples/long_lived_framework.cpp 2a79dfd3f81e5254922895af45c2b72d80ce5f49
> src/examples/no_executor_framework.cpp 7d841c6f364e1b671ec829aa9bff3b1f8ecf55ef
> src/examples/persistent_volume_framework.cpp 17140294a1eaeeeb92e9e14fc8638182b3a0682e
> src/examples/test_framework.cpp 9dbc18b039ee13cc1ec9454bd41cb3cfe30d63b4
> src/examples/test_http_framework.cpp a0cd84ab6dfdd8d6ed3403c6e06fabae6d5b46c9
>
>
> Diff: https://reviews.apache.org/r/61111/diff/6/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>