Re: Review Request 61111: Extracted strings into constants in example frameworks.

2017-08-10 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6/#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)


Let's keep the original double blank line space.



src/examples/load_generator_framework.cpp
Line 44 (original), 44-45 (patched)


Extra blank line?



src/examples/long_lived_framework.cpp
Lines 100 (patched)


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


Let's keep the original double blank line



src/examples/long_lived_framework.cpp
Line 584 (original), 592 (patched)


This field is deprecated since Mesos 1.0. Let's remove it altogether.



src/examples/no_executor_framework.cpp
Lines 46 (patched)


Missing `;` at the end.



src/examples/persistent_volume_framework.cpp
Line 55 (original), 55-56 (patched)


Ditto



src/examples/test_framework.cpp
Lines 56 (patched)


Ditto



src/examples/test_framework.cpp
Lines 58 (patched)


Missing []?



src/examples/test_framework.cpp
Lines 225-227 (original), 231-233 (patched)


```
uri = 
  path::join(os::realpath(Path(argv[0]).dirname()).get(), 
EXECUTOR_BINARY);
``` 
?



src/examples/test_framework.cpp
Line 255 (original), 261 (patched)


Ditto



src/examples/test_http_framework.cpp
Lines 70 (patched)


Ditto



src/examples/test_http_framework.cpp
Lines 411-413 (original), 416-418 (patched)


Can it fit two lines now?



src/examples/test_http_framework.cpp
Line 459 (original), 464 (patched)


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/6/
> ---
> 
> (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/6/diff/6/
> 
> 
> Testing
> ---
> 
> $ make check
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61111: Extracted strings into constants in example frameworks.

2017-08-08 Thread Benjamin Bannier 


> On Aug. 8, 2017, 4:35 p.m., Benno Evers wrote:
> > src/examples/balloon_framework.cpp
> > Line 520 (original), 524 (patched)
> > 
> >
> > 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/6/#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/6/
> ---
> 
> (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/6/diff/6/
> 
> 
> Testing
> ---
> 
> $ make check
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61111: Extracted strings into constants in example frameworks.

2017-08-08 Thread Benno Evers 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6/#review182390
---


Ship it!





src/examples/balloon_framework.cpp
Line 520 (original), 524 (patched)


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/6/
> ---
> 
> (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/6/diff/6/
> 
> 
> Testing
> ---
> 
> $ make check
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61111: Extracted strings into constants in example frameworks.

2017-08-07 Thread Armand Grillet 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6/
---

(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/6/diff/5/

Changes: https://reviews.apache.org/r/6/diff/4-5/


Testing
---

$ make check


Thanks,

Armand Grillet

Re: Review Request 61111: Extracted strings into constants in example frameworks.

2017-07-31 Thread Armand Grillet 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6/
---

(Updated July 31, 2017, 9:22 p.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Included /r/61135 and /r/61136.


Summary (updated)
-

Extracted strings into constants in example frameworks.


Bugs: MESOS-7814
https://issues.apache.org/jira/browse/MESOS-7814


Repository: mesos


Description (updated)
---

This impacts the balloon, disk full, and docker no executor frameworks.


Diffs (updated)
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
  src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 
  src/examples/docker_no_executor_framework.cpp 
4a58f11fc8892f23ade1d8c872ab9b4fc580d478 


Diff: https://reviews.apache.org/r/6/diff/3/

Changes: https://reviews.apache.org/r/6/diff/2-3/


Testing
---

$ make check


Thanks,

Armand Grillet