Review Request 34645: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34645/ --- Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 Diff: https://reviews.apache.org/r/34645/diff/ Testing --- Thanks, haosdent huang
Review Request 34644: Update existing lambdas to meet style guide
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34644/ --- Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Bugs: MESOS-2670 https://issues.apache.org/jira/browse/MESOS-2670 Repository: mesos Description --- Update existing lambdas to meet style guide Diffs - 3rdparty/libprocess/src/tests/process_tests.cpp 67e582cc250a9767a389e2bd0cc68985477f3ffb Diff: https://reviews.apache.org/r/34644/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 33753: Update pthread and python autoconf macros for Mesos.
On May 18, 2015, 6:31 p.m., haosdent huang wrote:
m4/ax_python_devel.m4, line 268
https://reviews.apache.org/r/33753/diff/1/?file=947258#file947258line268
Could not understand why need add conf('SYSLIBS')) here? Could you
explain this? Thank you
James Peach wrote:
``ax_python_devel.m4`` is taken unmodified from the autoconf archive. It
looks like ``SYSLIBS`` was added in commit
[6745ad4069f2fab26d72f374079cb8b7a4cf350f
](http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=commitdiff;h=6745ad4069f2fab26d72f374079cb8b7a4cf350f)
got it, Thank you. :-)
- haosdent
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33753/#review84185
---
On May 1, 2015, 4:02 p.m., James Peach wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33753/
---
(Updated May 1, 2015, 4:02 p.m.)
Review request for mesos.
Bugs: MESOS-2659
https://issues.apache.org/jira/browse/MESOS-2659
Repository: mesos
Description
---
Replace acx_pthread with ax_pthread. Update ax_python_devel. Update
configure.ac to use the autoconf macros.
Diffs
-
configure.ac 589ae97d0432370b462576cd1985544564893999
m4/acx_pthread.m4 2cf20de144a11be2aa603b04ea511244191037b7
m4/ax_pthread.m4 PRE-CREATION
m4/ax_python_devel.m4 3463d1430e16b7c9722319271e693e56bf1d323d
Diff: https://reviews.apache.org/r/33753/diff/
Testing
---
Bootstrap and verify there are no autotools warnings. Tested on CentOS 7 and
Mac OS X 10.10.3.
Thanks,
James Peach
Re: Review Request 34193: Refactored common functionality into BaseFlags
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34193/#review85098
---
Ship it!
Everything looks good but let's please resolve the issue with 'printUsage' and
then I'll commit.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
https://reviews.apache.org/r/34193/#comment136570
Adhering to the Google Style Guide, we do not use non-const reference
arguments to functions:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments
As the style guide points out, there are exceptions here, most notably
'operator ' which, by no coincidence, is relevant here and perhaps why this
slipped through? ;-)
Here's my suggestion to resolve this issue: let's add the 'std::string
message' to 'usage()'. There wasn't anything special about that function that
didn't include it originally, so I don't see any reason to not add it now.
Here's how this would look:
std::string usage(const Optionstd::string message = None()) const;
Now in the body of 'usage' we can add our default message or replace with
whatever someone else passed in. And then here is how it would look like when
used:
if (flags.help) {
std::cerr flags.usage(This is: TestFlags [options]) std::endl;
}
The alternative here would be to take std::ostream as a pointer, but where
ever we can cleanly prefer the functional style we should as it leads to more
composable and easier to reason about code (i.e., avoiding non-local
manipulation of variables, etc).
Moreover, I actually think it yields cleaner code at the call sites where
we are already using std::cerr directly, for example:
if (flags.master.isNone()) {
std::cerr Missing required option --master std::endl;
flags.printUsage(std::cerr);
return EXIT_FAILURE;
}
Would be cleaner to me as:
if (flags.master.isNone()) {
std::cerr Missing required option --master std::endl
flags.usage() std::endl;
return EXIT_FAILURE;
}
Finally, to cover all the cases here, I could imagine killing 'setUsageMsg'
and letting folks define their own constants that they always pass in, for
example:
const string USAGE_MESSAGE = This is: TestFlags [Options];
...
std::cerr flags.usage(USAGE_MESSAGE) std::endl;
To be clear I'm not opposed to having a usage message setter, but I'm
always in favor of having less ways, or ideally one way, of doing the same
thing. And regardless, s/setUsageMsg/setUsageMessage/ (it looks like Joris
mentioned this in a previous review, so just following up here, thanks guys!).
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
https://reviews.apache.org/r/34193/#comment136572
I think it would be great if we could consistently use the trailing
underscore it for any single class, rather than some fields using it and some
not as this could confuse people when to use it. In this case, any reason not
to change all the fields or not use the trailing underscore for now?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
https://reviews.apache.org/r/34193/#comment136574
Any reason not to make 'programName_' be an Option rather than setting it
to the empty string if argv[0] doesn't exist? While I understand in this case
we'd probably print the same thing, we definitely have more information with an
Option versus someone passing us an empty string for argv[0].
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
https://reviews.apache.org/r/34193/#comment136575
Two newlines between all top-level definitions/declarations please! I see
Joris called this out in a previous review but he just addressed a single place
instead of all the places. ;-) Thanks guys!
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
https://reviews.apache.org/r/34193/#comment136576
With the revised 'usage()' this will just be:
out flags.usage(This is: TestFlags [options]);
- Benjamin Hindman
On May 20, 2015, 11:22 p.m., Marco Massenzio wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34193/
---
(Updated May 20, 2015, 11:22 p.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description
---
Jira: MESOS-2711
Every program that uses stout's `BaseFlags` ends up
re-implementing the `printUsage()` function, and adding
a `bool help` (and associated --help flag); this functionality
has now been refactored in the base class and is
Re: Review Request 29889: Recover Docker containers when mesos slave is in a container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/ --- (Updated May 24, 2015, 6:26 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2115 https://issues.apache.org/jira/browse/MESOS-2115 Repository: mesos Description --- This is a one mega patch and contains many reviews that's already on rb. This review is not meant to be merged, only provided for easier review. Diffs (updated) - Dockerfile 35abf25 docs/configuration.md 54c4e31 docs/docker-containerizer.md a5438b7 src/Makefile.am 34755cf src/docker/docker.hpp 3ebbc1f src/docker/docker.cpp 3a485a2 src/docker/executor.hpp PRE-CREATION src/docker/executor.cpp PRE-CREATION src/slave/constants.hpp fd1c1ab src/slave/constants.cpp 2a99b11 src/slave/containerizer/docker.hpp ed4ee19 src/slave/containerizer/docker.cpp 408a443 src/slave/flags.hpp 5c57478 src/slave/flags.cpp b5e2518 src/tests/docker_containerizer_tests.cpp 154bf98 src/tests/docker_tests.cpp 5520c58 Diff: https://reviews.apache.org/r/29889/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave is in a container
On April 28, 2015, 10:11 p.m., Bernd Mathiske wrote: Dockerfile, line 18 https://reviews.apache.org/r/29889/diff/6/?file=940077#file940077line18 It seems that docker.io could be on its own line, since it is a kinda separate topic, which seems to be the grouping criterium here? I'll revert this since it's not reall related. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/#review81873 --- On May 24, 2015, 6:26 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/ --- (Updated May 24, 2015, 6:26 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2115 https://issues.apache.org/jira/browse/MESOS-2115 Repository: mesos Description --- This is a one mega patch and contains many reviews that's already on rb. This review is not meant to be merged, only provided for easier review. Diffs - Dockerfile 35abf25 docs/configuration.md 54c4e31 docs/docker-containerizer.md a5438b7 src/Makefile.am 34755cf src/docker/docker.hpp 3ebbc1f src/docker/docker.cpp 3a485a2 src/docker/executor.hpp PRE-CREATION src/docker/executor.cpp PRE-CREATION src/slave/constants.hpp fd1c1ab src/slave/constants.cpp 2a99b11 src/slave/containerizer/docker.hpp ed4ee19 src/slave/containerizer/docker.cpp 408a443 src/slave/flags.hpp 5c57478 src/slave/flags.cpp b5e2518 src/tests/docker_containerizer_tests.cpp 154bf98 src/tests/docker_tests.cpp 5520c58 Diff: https://reviews.apache.org/r/29889/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave is in a container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/ --- (Updated May 25, 2015, 5:25 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2115 https://issues.apache.org/jira/browse/MESOS-2115 Repository: mesos Description --- This is a one mega patch and contains many reviews that's already on rb. This review is not meant to be merged, only provided for easier review. Diffs (updated) - Dockerfile 35abf25 docs/configuration.md 54c4e31 docs/docker-containerizer.md a5438b7 src/Makefile.am 34755cf src/docker/docker.hpp 3ebbc1f src/docker/docker.cpp 3a485a2 src/docker/executor.hpp PRE-CREATION src/docker/executor.cpp PRE-CREATION src/slave/constants.hpp fd1c1ab src/slave/constants.cpp 2a99b11 src/slave/containerizer/docker.hpp ed4ee19 src/slave/containerizer/docker.cpp 408a443 src/slave/flags.hpp 5c57478 src/slave/flags.cpp b5e2518 src/tests/docker_containerizer_tests.cpp 154bf98 src/tests/docker_tests.cpp 5520c58 Diff: https://reviews.apache.org/r/29889/diff/ Testing --- make check Thanks, Timothy Chen
