Re: Review Request 33271: Update style guide to disallow capturing temporaries by reference.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33271/#review86765 --- Thanks guys! docs/mesos-c++-style-guide.md https://reviews.apache.org/r/33271/#comment138833 Do we really want to encourage taking a const of a POD type? In general we have not been doing this, so it seems pretty inconsistent to put it in this example. Also, looks like we need a space before the openening parenthesis. - Ben Mahler On June 2, 2015, 9:34 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33271/ --- (Updated June 2, 2015, 9:34 a.m.) Review request for mesos, Bernd Mathiske, Cody Maloney, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: MESOS-2629 https://issues.apache.org/jira/browse/MESOS-2629 Repository: mesos Description --- Follow up from r32630. Diffs - docs/mesos-c++-style-guide.md 13312f6f4fe1788791479bd768f60df0a8e80e69 Diff: https://reviews.apache.org/r/33271/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/#review86783 --- Patch looks great! Reviews applied: [32961, 33159] All tests passed. - Mesos ReviewBot On June 5, 2015, 8:53 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated June 5, 2015, 8:53 a.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs - include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/#review86767 --- Bad patch! Reviews applied: [34968, 34969, 34970] Failed command: ./support/apply-review.sh -n -r 34970 Error: 2015-06-05 06:24:06 URL:https://reviews.apache.org/r/34970/diff/raw/ [16370/16370] - 34970.patch [1] error: patch failed: src/examples/no_executor_framework.cpp:16 error: src/examples/no_executor_framework.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On June 5, 2015, 5:52 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/ --- (Updated June 5, 2015, 5:52 a.m.) Review request for mesos, Jie Yu and Vinod Kone. Bugs: MESOS-2655 https://issues.apache.org/jira/browse/MESOS-2655 Repository: mesos Description --- Now the NoExecutorScheduler can take a custom command, as well as the resources for the task and how many tasks to run. This allows us to continue using it for an end-to-end test as part of make check, but also allows it to be used against a cluster in a long-lived fashion. Diffs - src/examples/no_executor_framework.cpp 37001c389f31f9f1dafe6d7f3eb17adc2e369057 src/tests/no_executor_framework_test.sh d2d395595b778bc543e1baaa0fd415dc622b647f src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 Diff: https://reviews.apache.org/r/34970/diff/ Testing --- The existing no executor framework test picks this up. Since we do not have an non-zero estimator yet, I modified the slave to send revocable resources in order to manually test the revocable case. Thanks, Ben Mahler
Re: Review Request 33271: Update style guide to disallow capturing temporaries by reference.
Hey BenM,
BenH added spaces before the commit, but thanks for also catching it!
I'll follow up with a small patch to change the POD to something like
SlaveID.
Can you shepherd that one? :-)
Joris
On Fri, Jun 5, 2015 at 8:03 AM, Ben Mahler benjamin.mah...@gmail.com
wrote:
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33271/
Thanks guys!
docs/mesos-c++-style-guide.md
https://reviews.apache.org/r/33271/diff/9/?file=976428#file976428line180
(Diff
revision 9)
180
foreachpair(const int key, hashsetint values, index) {}
181
foreachvalue(const hashsetint values, index) {}
182
foreachkey(const int key, index) {}
Do we really want to encourage taking a const of a POD type? In general we
have not been doing this, so it seems pretty inconsistent to put it in this
example.
Also, looks like we need a space before the openening parenthesis.
- Ben Mahler
On June 2nd, 2015, 9:34 a.m. UTC, Joris Van Remoortere wrote:
Review request for mesos, Bernd Mathiske, Cody Maloney, Joerg Schad,
Michael Park, and Till Toenshoff.
By Joris Van Remoortere.
*Updated June 2, 2015, 9:34 a.m.*
*Bugs: * MESOS-2629 https://issues.apache.org/jira/browse/MESOS-2629
*Repository: * mesos
Description
Follow up from r32630.
Diffs
- docs/mesos-c++-style-guide.md
(13312f6f4fe1788791479bd768f60df0a8e80e69)
View Diff https://reviews.apache.org/r/33271/diff/
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/#review86781 --- Ship it! src/master/allocator/mesos/hierarchical.hpp https://reviews.apache.org/r/33159/#comment138859 CHECK_EQ - Benjamin Hindman On June 5, 2015, 8:53 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated June 5, 2015, 8:53 a.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs - include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 35090: Update libprocess process to use synchronized.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35090/ --- (Updated June 5, 2015, 7:42 a.m.) Review request for mesos, Benjamin Hindman and Bernd Mathiske. Changes --- remove extra friendship. Bugs: MESOS-2805 https://issues.apache.org/jira/browse/MESOS-2805 Repository: mesos Description --- See summary. Diffs (updated) - 3rdparty/libprocess/include/process/process.hpp 79d1719932a3fdc90b6247d3a77adee123e72435 3rdparty/libprocess/src/process.cpp d1b4d469a11abc618c1406bce602300dd9793b58 Diff: https://reviews.apache.org/r/35090/diff/ Testing --- Thanks, Joris Van Remoortere
Review Request 35120: Use non-POD type for alias example in c++ style guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35120/ --- Review request for mesos, Benjamin Hindman and Ben Mahler. Repository: mesos Description --- Also got rid of an unused struct. Diffs - docs/mesos-c++-style-guide.md 38dd201a22dd775971fad190378d022c50885969 Diff: https://reviews.apache.org/r/35120/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated June 5, 2015, 8:53 a.m.) Review request for mesos and Benjamin Hindman. Changes --- rebased. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs (updated) - include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 35102: Remove common/lock. Use synchronized instead.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35102/#review86779 --- Patch looks great! Reviews applied: [35088, 35089, 35090, 35091, 35092, 35093, 35094, 35095, 35096, 35097, 35098, 35099, 35100, 35101, 35102] All tests passed. - Mesos ReviewBot On June 5, 2015, 7:42 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35102/ --- (Updated June 5, 2015, 7:42 a.m.) Review request for mesos, Benjamin Hindman and Bernd Mathiske. Bugs: MESOS-2805 https://issues.apache.org/jira/browse/MESOS-2805 Repository: mesos Description --- See summary. Diffs - src/Makefile.am 3cf8bd2e86ce5823160f2984738c4e1391081c6a src/common/lock.hpp 988dff524e3d9f6c0bafa3398f6c3c258829cfe3 src/common/lock.cpp bb8ea3ada6b710357e6872959868d2d8a2035371 Diff: https://reviews.apache.org/r/35102/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 35000: Doxygen'ized Subprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35000/#review86787 --- Ship it! 3rdparty/libprocess/include/process/subprocess.hpp https://reviews.apache.org/r/35000/#comment138867 How about starting both enumerations with lower case here to make them part of the same sentence? 3rdparty/libprocess/include/process/subprocess.hpp https://reviews.apache.org/r/35000/#comment138868 This leading sentence can exist without the colon and hence the following enumerations could use capitalized sentence starts. I would replace the colon with a period to enable full sentences below. - Till Toenshoff On June 3, 2015, 1:45 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35000/ --- (Updated June 3, 2015, 1:45 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 37cab7755d2890619b64e1ca09e0b7ad0e72cf76 Diff: https://reviews.apache.org/r/35000/diff/ Testing --- make check doxygen ../Doxyfile Thanks, Benjamin Hindman
Re: Review Request 33271: Update style guide to disallow capturing temporaries by reference.
Here is the review. I used std::string instead. Also cleaned up an unused
struct.
https://reviews.apache.org/r/35120
On Fri, Jun 5, 2015 at 9:58 AM, Joris Van Remoortere
joris.van.remoort...@gmail.com wrote:
Hey BenM,
BenH added spaces before the commit, but thanks for also catching it!
I'll follow up with a small patch to change the POD to something like
SlaveID.
Can you shepherd that one? :-)
Joris
On Fri, Jun 5, 2015 at 8:03 AM, Ben Mahler benjamin.mah...@gmail.com
wrote:
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33271/
Thanks guys!
docs/mesos-c++-style-guide.md
https://reviews.apache.org/r/33271/diff/9/?file=976428#file976428line180
(Diff
revision 9)
180
foreachpair(const int key, hashsetint values, index) {}
181
foreachvalue(const hashsetint values, index) {}
182
foreachkey(const int key, index) {}
Do we really want to encourage taking a const of a POD type? In general
we have not been doing this, so it seems pretty inconsistent to put it in
this example.
Also, looks like we need a space before the openening parenthesis.
- Ben Mahler
On June 2nd, 2015, 9:34 a.m. UTC, Joris Van Remoortere wrote:
Review request for mesos, Bernd Mathiske, Cody Maloney, Joerg Schad,
Michael Park, and Till Toenshoff.
By Joris Van Remoortere.
*Updated June 2, 2015, 9:34 a.m.*
*Bugs: * MESOS-2629 https://issues.apache.org/jira/browse/MESOS-2629
*Repository: * mesos
Description
Follow up from r32630.
Diffs
- docs/mesos-c++-style-guide.md
(13312f6f4fe1788791479bd768f60df0a8e80e69)
View Diff https://reviews.apache.org/r/33271/diff/
Re: Review Request 34256: Added Path::dirname() and Path::basename().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34256/ --- (Updated June 5, 2015, 1:03 p.m.) Review request for mesos and Cody Maloney. Changes --- Some simplification, added more comments and did a rebase. Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 Diff: https://reviews.apache.org/r/34256/diff/ Testing --- make check (including new tests). Result comparison to match ::dirname and ::basename on interesting cases. Thanks, Till Toenshoff
Re: Review Request 34259: Replaced os::dirname and os::basename with Path::dirname and Path::basename.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34259/ --- (Updated June 5, 2015, 1:22 p.m.) Review request for mesos and Cody Maloney. Changes --- rebased Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- see summary. Diffs (updated) - 3rdparty/libprocess/src/process.cpp f1d3e5b Diff: https://reviews.apache.org/r/34259/diff/ Testing --- make check on trailing RR. Thanks, Till Toenshoff
Re: Review Request 34256: Added Path::dirname() and Path::basename().
On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 29
https://reviews.apache.org/r/34256/diff/4/?file=962202#file962202line29
nice tests.
Can I drop this issue :)?
On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 32-90
https://reviews.apache.org/r/34256/diff/4/?file=962201#file962201line32
Can you add comments to the code here? It is really hard to follow
what's happening.
Alternatively, can you simplify these using strings functions?
For example, looking at http://linux.die.net/man/3/basename
```
The functions dirname() and basename() break a null-terminated pathname
string into directory and filename components. In the usual case, dirname()
returns the string up to, but not including, the final '/', and basename()
returns the component following the final '/'. Trailing '/' characters are
not counted as part of the pathname.
If path does not contain a slash, dirname() returns the string .
while basename() returns a copy of path. If path is the string /, then
both dirname() and basename() return the string /. If path is a NULL
pointer or points to an empty string, then both dirname() and basename()
return the string ..
```
this is how I would implement basename
```
string basename()
{
// Remove trailing /, if exists.
string result = strings::remove(value, /, strings::SUFFIX);
if (result.empty()) {
return string(.);
}
if (!strings::contains(result, /)) {
return result;
}
if (result == /) {
return result;
}
vectorstring tokens = strings::tokenize(result, /);
return tokens[tokens.size() - 1];
}
```
I haven't checked all the edge cases, but you get the idea.
I thought a while about how we could use strings::remove but unfortunately I
did not see a good way to make sure that e.g. multiple trailing slashes are
properly cut off. In the end I decided that using an indexed based parsing
would be more efficient and by the added comments also readable fine - at least
that is what I think :)
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review84375
---
On May 17, 2015, 7:46 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/
---
(Updated May 17, 2015, 7:46 p.m.)
Review request for mesos and Cody Maloney.
Bugs: MESOS-1303
https://issues.apache.org/jira/browse/MESOS-1303
Repository: mesos-incubating
Description
---
Introducing Path::dirname() and Path::basename() as a thread safe replacement
of the respective system functions. Also contains new tests covering corner
cases.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
Diff: https://reviews.apache.org/r/34256/diff/
Testing
---
make check (including new tests).
Result comparison to match ::dirname and ::basename on interesting cases.
Thanks,
Till Toenshoff
Review Request 35129: Refactor Future::Data to use ResultT. Remove dynamic allocation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/ --- Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-2801 https://issues.apache.org/jira/browse/MESOS-2801 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/future.hpp 75cbe122f18d2a8ea919331e7fc91e1ec3bfd28b Diff: https://reviews.apache.org/r/35129/diff/ Testing --- make check Thanks, Joris Van Remoortere
Review Request 35131: Replaced os::dirname and os::basename with Path::dirname and Path::basename.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35131/ --- Review request for mesos and Cody Maloney. Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da Diff: https://reviews.apache.org/r/35131/diff/ Testing --- make check on trailing RR. Thanks, Till Toenshoff
Re: Review Request 34943: Added validation to flags.
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 141
https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line141
can you please add some documentation to explain what each type (and
the relative @param) is?
(using Doxygen would be awesome :)
Most definitely, but I'll do that in another review.
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 146
https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line146
How do we enforce that F is a function, and not, say, a std::string?
It gets enforced by using 'F'as a function in the body of 'add'. Bottom line,
we take any functor here, if we can invoke the apply operator than we're good
to go.
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line184
too much choice, IMO - there are (if I counted them right) 8 different
`add()` variants to choose from (none of them documented :)
It's virtually impossible for folks to figure out which to pick and
this leads, I believe, to the current 'code by copying' approach.
I would cull it down to no more than 2-3 different options, with some
sensible default values.
I completely agree this needs documentation, which I'll take on in a different
review. But there are really only 4 variants here:
(1) The flag _is_ a class member.
(A) The flag has a default value (i.e., is _not_ an OptionT).
(B) The flag does not have a default value (i.e., _is_ an OptionT).
(2) The flag is _not_ a class member.
(A) The flag has a default value (i.e., is _not_ an OptionT).
(B) The flag does not have a default value (i.e., _is_ an OptionT).
All 4 of these options can optionally have a validation function, but since we
can't capture the default validation function with the existing 4 functions we
have to do the delegating function trick, resulting in 8 functions. Bottom
line, this needs to be documented (and if someone has a better suggestion to
avoid the delgating function trick I'm all ears).
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line500
so, with your approach, everyone, every time that they add a required
flag, will have to, essentially, copy paste this code.
what I had envisioned, was a simpler way (eg, by adding a `bool
required = false` arg) that, if true would automatically do this validation
step in `FlagsBase`
(of course, we could add another `add()` method that has that
signature, that just internally implements this and calls one of the other
`add()` ones - making it the ninth option :) )
I wasn't trying to solve the required flag problem, I was just trying to show
an example of a validation function. But I violently agree that this is
therefore a horribly bad example that sets a bad precedent, so I've replaced
this with a different validation function.
I also agree that we should introduce a simplier way, such as a bool as you
suggested (or better an an enumeration). This doesn't completley solve the
problem, however, since a flag without a default value will still be an Option
and still require one to do 'CHECK_SOME(flags.flag)'. Got any other ideas?
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line514
I'll admit I'm still a bith hazy about the new { } initializer, but
could have this been:
```
char* argv[] = { (char*) /path/to/program,
(char*) --name1=billy joel };
```
I just copied this code. Let me give this a test and if so I'll cleanup the
others too.
- Benjamin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
---
On June 2, 2015, 2:43 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
---
(Updated June 2, 2015, 2:43 p.m.)
Review request for mesos.
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp
87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
Re: Review Request 34260: Replaced os::dirname and os::basename with Path::dirname and Path::basename.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34260/ --- (Updated June 5, 2015, 1:26 p.m.) Review request for mesos and Cody Maloney. Changes --- Rebased. Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- see summary. Diffs (updated) - src/cli/mesos.cpp 1121e19 src/cli/resolve.cpp 74545a0 src/examples/low_level_scheduler_libprocess.cpp df92e8d src/examples/low_level_scheduler_pthread.cpp 175ee4d src/examples/persistent_volume_framework.cpp ee2311f src/examples/test_framework.cpp 25f5f8c src/files/files.cpp ce02411 src/health-check/main.cpp 3607479 src/launcher/executor.cpp f79dc60 src/linux/cgroups.cpp 831237b src/local/main.cpp ec21ed0 src/logging/logging.cpp 6b14575 src/slave/containerizer/fetcher.cpp f77652b src/slave/containerizer/isolators/cgroups/cpushare.cpp 5bd3525 src/slave/containerizer/isolators/cgroups/mem.cpp 9647e79 src/slave/containerizer/isolators/cgroups/perf_event.cpp 3e5153f src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf src/slave/containerizer/linux_launcher.cpp 8eae258 src/slave/main.cpp c4d8940 src/slave/state.hpp fed4b7e src/slave/state.cpp 8eda22a src/slave/status_update_manager.cpp 1d7c4d0 src/tests/fetcher_tests.cpp 361d918 src/tests/mesos.cpp d3a8bb7 src/zookeeper/group.cpp 173caa8 Diff: https://reviews.apache.org/r/34260/diff/ Testing --- make check Thanks, Till Toenshoff
Review Request 35125: Fixed verb naming of Flag::loader to Flag::load.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35125/ --- Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb Diff: https://reviews.apache.org/r/35125/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35127: Updated libprocess to match stout Flags changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35127/ --- Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068 Diff: https://reviews.apache.org/r/35127/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35128: Updated Mesos to match stout Flags changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35128/ --- Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5 src/slave/http.cpp bc25bdd33277dbfa30410ad081ea09f0fc39c598 Diff: https://reviews.apache.org/r/35128/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35126: Used C++11 lambdas instead of functors in FlagsBase.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35126/ --- Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984 3rdparty/libprocess/3rdparty/stout/tests/subcommand_tests.cpp c40bba4f1e7eef7cb04f79b567e32684648b2004 Diff: https://reviews.apache.org/r/35126/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated June 5, 2015, 1:35 p.m.) Review request for mesos and Benjamin Hindman. Changes --- switched CHECK to CHECK_EQ Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs (updated) - include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 35125: Fixed verb naming of Flag::loader to Flag::load.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35125/ --- (Updated June 5, 2015, 2:21 p.m.) Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb Diff: https://reviews.apache.org/r/35125/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
On June 5, 2015, 5:42 p.m., Vinod Kone wrote: src/master/allocator/mesos/hierarchical.hpp, lines 455-456 https://reviews.apache.org/r/33159/diff/4/?file=979945#file979945line455 I don't follow. Why are these CHECKs? Is there currently code in the master that guarantees that these checks won't fail? actually nm. looks like this is being called with framework-info in the master and not with the frameworkinfo that a framework re-registers with. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/#review86818 --- On June 5, 2015, 1:35 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated June 5, 2015, 1:35 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs - include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86808 --- src/common/http.hpp https://reviews.apache.org/r/34687/#comment138901 s/info/masterInfo/ src/common/http.cpp https://reviews.apache.org/r/34687/#comment138902 Unfold to: ``` /** * Foobar */ ``` src/common/parse.hpp https://reviews.apache.org/r/34687/#comment138903 Should this block have 2 space indent or 1? You have 1 above :) src/tests/common/parse_tests.cpp https://reviews.apache.org/r/34687/#comment138899 ASSERT_EQ() on the master infos instead of SerializeAsString()? - Niklas Nielsen On June 3, 2015, 2 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 2 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/#review86818 --- src/master/allocator/mesos/hierarchical.hpp https://reviews.apache.org/r/33159/#comment138910 I don't follow. Why are these CHECKs? Is there currently code in the master that guarantees that these checks won't fail? - Vinod Kone On June 5, 2015, 1:35 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated June 5, 2015, 1:35 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs - include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/tests/mesos.hpp 86660ac4d7476bc6cc077fc2d474c4a9ac81c031 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 --- I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. src/common/http.cpp https://reviews.apache.org/r/34687/#comment138912 Why do you need this temporary? src/common/parse.hpp https://reviews.apache.org/r/34687/#comment138920 s/error out/return an error/ src/common/parse.hpp https://reviews.apache.org/r/34687/#comment138921 s/result/info/ src/common/parse.hpp https://reviews.apache.org/r/34687/#comment138923 ips sounds like multiple IPs though i know you meant it as IP as a string. just call it ipString src/common/parse.hpp https://reviews.apache.org/r/34687/#comment138922 s/this/This/ - Vinod Kone On June 3, 2015, 9 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 9 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 11:42 a.m., Vinod Kone wrote: src/common/http.cpp, line 212 https://reviews.apache.org/r/34687/diff/6/?file=977637#file977637line212 Why do you need this temporary? BenH brought this up - look above. I think this is fine if it is made const. - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 --- On June 3, 2015, 2 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 2 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 34258: Removed os::dirname and os::basename.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34258/ --- (Updated June 5, 2015, 4:39 p.m.) Review request for mesos and Cody Maloney. Changes --- re-triggering the buildbot Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- os::dirname and os::basename were not thread safe on OSX. Replacements are path::dirname and path::basename. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/README.md 588f739 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be Diff: https://reviews.apache.org/r/34258/diff/ Testing --- make check on trailing RR. Thanks, Till Toenshoff
Re: Review Request 35129: Refactor Future::Data to use ResultT. Remove dynamic allocation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/#review86812 --- Patch looks great! Reviews applied: [35129] All tests passed. - Mesos ReviewBot On June 5, 2015, 1:16 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/ --- (Updated June 5, 2015, 1:16 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-2801 https://issues.apache.org/jira/browse/MESOS-2801 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/future.hpp 75cbe122f18d2a8ea919331e7fc91e1ec3bfd28b Diff: https://reviews.apache.org/r/35129/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/ --- (Updated June 5, 2015, 7:56 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Changes --- Resolved merge conflicts for the os::getenv changes that landed. Bugs: MESOS-2655 https://issues.apache.org/jira/browse/MESOS-2655 Repository: mesos Description --- Now the NoExecutorScheduler can take a custom command, as well as the resources for the task and how many tasks to run. This allows us to continue using it for an end-to-end test as part of make check, but also allows it to be used against a cluster in a long-lived fashion. Diffs (updated) - src/examples/no_executor_framework.cpp 1fb853d6e4a3deb3c36e6e661689697c8ebf898f src/tests/no_executor_framework_test.sh d2d395595b778bc543e1baaa0fd415dc622b647f src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 Diff: https://reviews.apache.org/r/34970/diff/ Testing --- The existing no executor framework test picks this up. Since we do not have an non-zero estimator yet, I modified the slave to send revocable resources in order to manually test the revocable case. Thanks, Ben Mahler
Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34970/#review86846
---
Ship it!
src/examples/no_executor_framework.cpp
https://reviews.apache.org/r/34970/#comment138944
I think it is easier to follow if this is written as:
```
while(remaining.contains(taskResources)
(totalTasks.isNone() || tasksLaunched totalTasks.get())) {
}
```
src/examples/no_executor_framework.cpp
https://reviews.apache.org/r/34970/#comment138946
LOG(FATAL)? because this is not possible with a command executor?
src/examples/no_executor_framework.cpp
https://reviews.apache.org/r/34970/#comment138948
s/active/launched/ ?
src/examples/no_executor_framework.cpp
https://reviews.apache.org/r/34970/#comment138943
Also mention that, if this is not specified, as many tasks as possible are
launched?
src/examples/no_executor_framework.cpp
https://reviews.apache.org/r/34970/#comment138942
Only set this if flags.task_revocable_resources.isSome() ?
- Vinod Kone
On June 5, 2015, 7:56 p.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34970/
---
(Updated June 5, 2015, 7:56 p.m.)
Review request for mesos, Jie Yu and Vinod Kone.
Bugs: MESOS-2655
https://issues.apache.org/jira/browse/MESOS-2655
Repository: mesos
Description
---
Now the NoExecutorScheduler can take a custom command, as well as the
resources for the task and how many tasks to run.
This allows us to continue using it for an end-to-end test as part of make
check, but also allows it to be used against a cluster in a long-lived
fashion.
Diffs
-
src/examples/no_executor_framework.cpp
1fb853d6e4a3deb3c36e6e661689697c8ebf898f
src/tests/no_executor_framework_test.sh
d2d395595b778bc543e1baaa0fd415dc622b647f
src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51
Diff: https://reviews.apache.org/r/34970/diff/
Testing
---
The existing no executor framework test picks this up.
Since we do not have an non-zero estimator yet, I modified the slave to send
revocable resources in order to manually test the revocable case.
Thanks,
Ben Mahler
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 6:42 p.m., Vinod Kone wrote: I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. Niklas Nielsen wrote: We have been working on this for a while now and we are using JSON::Protobuf() aldready and can enhance it further with your suggestion. However, the current approach isn't semantically different from the one you suggest and we would like to move forward and make that change later. Is that OK with you? Hey Vinod - as Niklas pointed out, we have invested a significant amount of time on this one, including the manual testing I've done (as summarized on MESOS-2304) and I'd be really reluctant to start again from scratch. As I don't really think there is any semantic difference; my approach does not introduce any performance penalty (in fact, I believe it may even be better than a 'generic' one); and, finally, that this does not impact the readability of the code, we should commit the code 'as is' (with your suggestions below) and move on. There are a ton of features and work to do, and I'd like us to focus on important issues. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 --- On June 3, 2015, 9 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 9 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35152: Document and consolidate qdisc handles
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/#review86847 --- Patch looks great! Reviews applied: [35150, 35152] All tests passed. - Mesos ReviewBot On June 5, 2015, 8:38 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/ --- (Updated June 5, 2015, 8:38 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Document and consolidate qdisc handles Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 Diff: https://reviews.apache.org/r/35152/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 6:42 p.m., Vinod Kone wrote: I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. Niklas Nielsen wrote: We have been working on this for a while now and we are using JSON::Protobuf() aldready and can enhance it further with your suggestion. However, the current approach isn't semantically different from the one you suggest and we would like to move forward and make that change later. Is that OK with you? Marco Massenzio wrote: Hey Vinod - as Niklas pointed out, we have invested a significant amount of time on this one, including the manual testing I've done (as summarized on MESOS-2304) and I'd be really reluctant to start again from scratch. As I don't really think there is any semantic difference; my approach does not introduce any performance penalty (in fact, I believe it may even be better than a 'generic' one); and, finally, that this does not impact the readability of the code, we should commit the code 'as is' (with your suggestions below) and move on. There are a ton of features and work to do, and I'd like us to focus on important issues. Vinod Kone wrote: Marco. I appreciate that you invested significant effort to making sure the backwards compatibility story is airtight, but I urge you to spend some extra cycles to simplify the code and avoid tech debt. I honestly don't think it would take too much time to simplify this. I would really like to understand what's the most time consuming part here? The compatibility testing? Can you automate it with niklas's compatibility script? As an aside, going through earlier reviews I saw that BenH made similar comments but it got sidetracked with deprecating ip. Also, my fault for not jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I would be happy to shepherd this change for you going forward. The upgrade/change of MasterInfo is tracked in https://issues.apache.org/jira/browse/MESOS-2736 which I believe is a different topic than what is addressed here (which is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll update this patch description in a sec). In any event, as `ip` is a required int32 field, we MUST support it, no matter what - this patch does this, correctly, without introducing tech debt (I don't see where I'm doing that). Thanks in advance for your understanding. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 --- On June 3, 2015, 9 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 9 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 35152: Document and consolidate qdisc handles
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/ --- Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Document and consolidate qdisc handles Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 Diff: https://reviews.apache.org/r/35152/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 11:42 a.m., Vinod Kone wrote: I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. We have been working on this for a while now and we are using JSON::Protobuf() aldready and can enhance it further with your suggestion. However, the current approach isn't semantically different from the one you suggest and we would like to move forward and make that change later. Is that OK with you? - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 --- On June 3, 2015, 2 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 2 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 35119: Introduced metrics for revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35119/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-2776 https://issues.apache.org/jira/browse/MESOS-2776 Repository: mesos Description --- state.json changes are in a subsequent review. Diffs - src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35119/diff/ Testing --- make check. - Modified a test to test the `total` resources metrics. - We don't have unit tests that use the revocable resources yet, when we add that we should check `used` resources metrics too. Thanks, Jiang Yan Xu
Review Request 35157: Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/ --- Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Repository: mesos Description --- Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor. This is the unit test for https://reviews.apache.org/r/34980/ Diffs - src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35157/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 35152: Document and consolidate qdisc handles
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/#review86843 --- src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138941 s/connection/interface/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138939 s/data/packet/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138940 s/policies/filters/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138937 s/interface qdisc/qdisc/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138938 Nit: there could be more than one filters since we could have more than one port ranges for each container. src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138931 What is filter chain?? There is no such thing in TC. Also we don't have any filter on htb. src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138932 Ditto, there is no filter here, just a class. src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138933 The fq_codel qdisc here is for reducing buffer-bloat, not traffic shaping. src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138934 s/FInally/Finally/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138935 Ditto, the fq_codel here is for traffic isolation and reducing buffer-bloat. - Cong Wang On June 5, 2015, 8:38 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/ --- (Updated June 5, 2015, 8:38 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Document and consolidate qdisc handles Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 Diff: https://reviews.apache.org/r/35152/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34984: Added help for files
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34984/#review86849 --- Ship it! I will make the change for you below. Thanks Aditi! src/files/files.cpp https://reviews.apache.org/r/34984/#comment138950 Should be: ``` using process::DESCRIPTION; using process::HELP; using process::TLDR; using process::USAGE; using process::wait; // Necessary on some OS's to disambiguate. ``` Underneath 'using namespace process;' :) src/files/files.cpp https://reviews.apache.org/r/34984/#comment138947 s/Path/path/ - Niklas Nielsen On June 4, 2015, 2:40 a.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34984/ --- (Updated June 4, 2015, 2:40 a.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2277 https://issues.apache.org/jira/browse/MESOS-2277 Repository: mesos Description --- Added help for files Diffs - src/files/files.cpp ce02411c5e579d7551b4325ec141fd89e4ee7255 Diff: https://reviews.apache.org/r/34984/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 6:42 p.m., Vinod Kone wrote: I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. Niklas Nielsen wrote: We have been working on this for a while now and we are using JSON::Protobuf() aldready and can enhance it further with your suggestion. However, the current approach isn't semantically different from the one you suggest and we would like to move forward and make that change later. Is that OK with you? Marco Massenzio wrote: Hey Vinod - as Niklas pointed out, we have invested a significant amount of time on this one, including the manual testing I've done (as summarized on MESOS-2304) and I'd be really reluctant to start again from scratch. As I don't really think there is any semantic difference; my approach does not introduce any performance penalty (in fact, I believe it may even be better than a 'generic' one); and, finally, that this does not impact the readability of the code, we should commit the code 'as is' (with your suggestions below) and move on. There are a ton of features and work to do, and I'd like us to focus on important issues. Marco. I appreciate that you invested significant effort to making sure the backwards compatibility story is airtight, but I urge you to spend some extra cycles to simplify the code and avoid tech debt. I honestly don't think it would take too much time to simplify this. I would really like to understand what's the most time consuming part here? The compatibility testing? Can you automate it with niklas's compatibility script? As an aside, going through earlier reviews I saw that BenH made similar comments but it got sidetracked with deprecating ip. Also, my fault for not jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I would be happy to shepherd this change for you going forward. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 --- On June 3, 2015, 9 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 3, 2015, 9 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated June 5, 2015, 8:18 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2807 https://issues.apache.org/jira/browse/MESOS-2807 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 src/tests/common/parse_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35150/#review86845 --- Ship it! Ship It! - Chi Zhang On June 5, 2015, 8:30 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35150/ --- (Updated June 5, 2015, 8:30 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Add output stream operation for handle to use in port_mapping.cpp Diffs - src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 Diff: https://reviews.apache.org/r/35150/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 35118: Made updateSlave() update its 'totalResources'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35118/#review86850 --- src/master/master.cpp https://reviews.apache.org/r/35118/#comment138949 woah. didn't realize this was handled automagically by the install handler. src/master/master.cpp https://reviews.apache.org/r/35118/#comment138951 while you are at it, can you just send slave-totalResources here instead of oversubscribedResources? this will address my TODO on this method. - Vinod Kone On June 5, 2015, 9:09 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35118/ --- (Updated June 5, 2015, 9:09 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2776 https://issues.apache.org/jira/browse/MESOS-2776 Repository: mesos Description --- - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources. - Changed updateSlave() argument to use `const Resources oversubscribedResources` instead of `const std::vectorResource oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`. Diffs - src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f src/master/master.cpp be0db42da3c59761aa154439653d715556465256 Diff: https://reviews.apache.org/r/35118/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.
On June 5, 2015, 6:24 a.m., Mesos ReviewBot wrote: Bad patch! Reviews applied: [34968, 34969, 34970] Failed command: ./support/apply-review.sh -n -r 34970 Error: 2015-06-05 06:24:06 URL:https://reviews.apache.org/r/34970/diff/raw/ [16370/16370] - 34970.patch [1] error: patch failed: src/examples/no_executor_framework.cpp:16 error: src/examples/no_executor_framework.cpp: patch does not apply Failed to apply patch Interesting, this shouldn't have applied the previous two reviews since [MESOS-1881](https://issues.apache.org/jira/browse/MESOS-1881) is resolved. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/#review86767 --- On June 5, 2015, 5:52 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/ --- (Updated June 5, 2015, 5:52 a.m.) Review request for mesos, Jie Yu and Vinod Kone. Bugs: MESOS-2655 https://issues.apache.org/jira/browse/MESOS-2655 Repository: mesos Description --- Now the NoExecutorScheduler can take a custom command, as well as the resources for the task and how many tasks to run. This allows us to continue using it for an end-to-end test as part of make check, but also allows it to be used against a cluster in a long-lived fashion. Diffs - src/examples/no_executor_framework.cpp 37001c389f31f9f1dafe6d7f3eb17adc2e369057 src/tests/no_executor_framework_test.sh d2d395595b778bc543e1baaa0fd415dc622b647f src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 Diff: https://reviews.apache.org/r/34970/diff/ Testing --- The existing no executor framework test picks this up. Since we do not have an non-zero estimator yet, I modified the slave to send revocable resources in order to manually test the revocable case. Thanks, Ben Mahler
Review Request 35118: Made updateSlave() update its 'totalResources'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35118/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-2776 https://issues.apache.org/jira/browse/MESOS-2776 Repository: mesos Description --- - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources. - Changed updateSlave() argument to use `const Resources oversubscribedResources` instead of `const std::vectorResource oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`. Diffs - src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f src/master/master.cpp be0db42da3c59761aa154439653d715556465256 Diff: https://reviews.apache.org/r/35118/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 35119: Introduced metrics for revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35119/#review86851 --- Ship it! src/master/metrics.hpp https://reviews.apache.org/r/35119/#comment138952 s/irrevocable/non-revocable/ src/master/metrics.cpp https://reviews.apache.org/r/35119/#comment138953 ditto. - Vinod Kone On June 5, 2015, 9:13 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35119/ --- (Updated June 5, 2015, 9:13 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2776 https://issues.apache.org/jira/browse/MESOS-2776 Repository: mesos Description --- state.json changes are in a subsequent review. Diffs - src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f src/master/master.cpp be0db42da3c59761aa154439653d715556465256 src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35119/diff/ Testing --- make check. - Modified a test to test the `total` resources metrics. - We don't have unit tests that use the revocable resources yet, when we add that we should check `used` resources metrics too. Thanks, Jiang Yan Xu
Re: Review Request 34943: Added validation to flags.
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line184
too much choice, IMO - there are (if I counted them right) 8 different
`add()` variants to choose from (none of them documented :)
It's virtually impossible for folks to figure out which to pick and
this leads, I believe, to the current 'code by copying' approach.
I would cull it down to no more than 2-3 different options, with some
sensible default values.
Benjamin Hindman wrote:
I completely agree this needs documentation, which I'll take on in a
different review. But there are really only 4 variants here:
(1) The flag _is_ a class member.
(A) The flag has a default value (i.e., is _not_ an OptionT).
(B) The flag does not have a default value (i.e., _is_ an OptionT).
(2) The flag is _not_ a class member.
(A) The flag has a default value (i.e., is _not_ an OptionT).
(B) The flag does not have a default value (i.e., _is_ an OptionT).
All 4 of these options can optionally have a validation function, but
since we can't capture the default validation function with the existing 4
functions we have to do the delegating function trick, resulting in 8
functions. Bottom line, this needs to be documented (and if someone has a
better suggestion to avoid the delgating function trick I'm all ears).
Why should the callers care if it has a default value, type-wise?
My thinking was, let's always use an `OptionT` for the flag; a, possibly
`None()`, default value and a `required` bool flag:
```
templatetypename T
void add(OptionT* option,
const std::string name,
const std::string help,
const OptionT const default = None(),
bool required = false)
```
and similarly for the class member.
So, all we have are *two* options, and no doubt as to which one to use.
I'm sure I'm missing a ton here, though... especially given that:
```
Optionstring foo;
Optionstring bar;
Optionstring def_foo{default-foo-val};
flags.add(foo, foo, this is foo!, def_foo, true);
flags.add1(bar, bar, this is BAR);
```
works as intended, but:
```
flags.add(foo, foo, this is foo!, default-foo-val, true);
```
causes a compile error (even though, I can totally see an `Option(const T _t)`
constructor that should just work (as it very much does for `def_foo`).
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line500
so, with your approach, everyone, every time that they add a required
flag, will have to, essentially, copy paste this code.
what I had envisioned, was a simpler way (eg, by adding a `bool
required = false` arg) that, if true would automatically do this validation
step in `FlagsBase`
(of course, we could add another `add()` method that has that
signature, that just internally implements this and calls one of the other
`add()` ones - making it the ninth option :) )
Benjamin Hindman wrote:
I wasn't trying to solve the required flag problem, I was just trying
to show an example of a validation function. But I violently agree that this
is therefore a horribly bad example that sets a bad precedent, so I've
replaced this with a different validation function.
I also agree that we should introduce a simplier way, such as a bool as
you suggested (or better an an enumeration). This doesn't completley solve
the problem, however, since a flag without a default value will still be an
Option and still require one to do 'CHECK_SOME(flags.flag)'. Got any other
ideas?
horribly bad example
I never said that :)
As mentioned above, a flag should *always* be an `OptionT` with the
difference that, if we had marked it as `required` then we'd be confident that,
once loaded, it does have a value (so, no need to `CHECK_SOME()`) or it would
have already failed.
For the optional (aka `non-required`) ones (without a default value) obviously
they could be `None()` but that is an expected outcome and the caller can
take whatever action they deem appropriate.
From the 'client' perspective:
1. I told you it was `required` -- there must be a value (or you'd have failed
before getting back to me)
2. I gave you a default value -- there must be a value (possibly the default
one)
3. It's neither required, nor has a default -- its absence (`isNone() ==
true`) has a meaning to me and I'll take appropriate action
4. It's required, and I gave you a default value -- I'm bad at logical
thinking ;-)
I would also propose (similarly to the --help case) to add an
`onMissingRequired()` hook with the default implementation
```
void onMissingRequired(const string name)
{
Re: Review Request 35131: Replaced os::dirname and os::basename with Path::dirname and Path::basename.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35131/#review86869 --- Ship it! Ship It! - Joerg Schad On June 5, 2015, 1:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35131/ --- (Updated June 5, 2015, 1:20 p.m.) Review request for mesos and Cody Maloney. Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da Diff: https://reviews.apache.org/r/35131/diff/ Testing --- make check on trailing RR. Thanks, Till Toenshoff
Review Request 35165: Extend fq_codel to allow parent and handle to be specified at runtime.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35165/ --- Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2332 https://issues.apache.org/jira/browse/MESOS-2332 Repository: mesos Description --- Extend fq_codel to allow parent and handle to be specified at runtime. Diffs - src/linux/routing/queueing/fq_codel.hpp 412af2d7da2c70324234ee75df75c71319b1365a src/linux/routing/queueing/fq_codel.cpp 3db5b9390bb5ae759eb1b7c9d1ac87cfca8d531e src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 src/tests/routing_tests.cpp db5b5df48634ff322baf9328fc605b2667b56eed Diff: https://reviews.apache.org/r/35165/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 35157: Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource Estimator .
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/#review86882 --- Patch looks great! Reviews applied: [35164, 35157] All tests passed. - Mesos ReviewBot On June 5, 2015, 11:53 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/ --- (Updated June 5, 2015, 11:53 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Repository: mesos Description --- Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor. This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/ Diffs - src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35157/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 35131: Replaced os::dirname and os::basename with Path::dirname and Path::basename.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35131/#review86864 --- Ship it! Ship It! - Vinod Kone On June 5, 2015, 1:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35131/ --- (Updated June 5, 2015, 1:20 p.m.) Review request for mesos and Cody Maloney. Bugs: MESOS-1303 https://issues.apache.org/jira/browse/MESOS-1303 Repository: mesos-incubating Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da Diff: https://reviews.apache.org/r/35131/diff/ Testing --- make check on trailing RR. Thanks, Till Toenshoff
Re: Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35150/ --- (Updated June 5, 2015, 11:33 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Incorporated reviewer feedback. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Add output stream operation for handle to use in port_mapping.cpp Diffs (updated) - src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 Diff: https://reviews.apache.org/r/35150/diff/ Testing --- make check Thanks, Paul Brett
Review Request 35164: Added callback to the QoS Controller to retrieve usages from the monitor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35164/ --- Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2823 https://issues.apache.org/jira/browse/MESOS-2823 Repository: mesos Description --- Passed callback to the QoS Controller to retrieve ResourceUsage from Resource Monitor on demand. This is neccessary, since QoS Controller needs data (current statistics for each executor) on which it will base its potential corrections. Diffs - include/mesos/slave/qos_controller.hpp 1d89acfd9c742b044674e0a0815f9f01eccb69b3 src/slave/qos_controller.hpp b37798303561eb79aee202b9c110794517eeed06 src/slave/qos_controller.cpp 81c4b3e658902be0438f42d9e86911e424828a73 src/slave/slave.cpp 054929b156374a8929ac9fffb032045f13c3eb43 src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 Diff: https://reviews.apache.org/r/35164/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 35157: Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/#review86859 --- Above ReviewBot message refers to the initial patch before ReviewRequest change! ): - Bartek Plotka On June 5, 2015, 10:52 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/ --- (Updated June 5, 2015, 10:52 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Repository: mesos Description --- Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor. This is the unit test for https://reviews.apache.org/r/34980/ Diffs - src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35157/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 35152: Document and consolidate qdisc handles
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/#review86870 --- src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment138998 s/data/packet/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment139002 s/filter/class/ src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/35152/#comment139004 s/CLASS_ID/HTB_CLASS_ID/ ? - Cong Wang On June 5, 2015, 11:55 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/ --- (Updated June 5, 2015, 11:55 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Document and consolidate qdisc handles Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 Diff: https://reviews.apache.org/r/35152/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 35157: Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/ --- (Updated June 5, 2015, 10:52 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Added forgotten files. Repository: mesos Description --- Added test for ResourceEstimator fetching ResoruceUsage from ResourceMonitor. This is the unit test for https://reviews.apache.org/r/34980/ Diffs (updated) - src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35157/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 35157: Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource Estimator .
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35157/ --- (Updated June 5, 2015, 11:53 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Added QoS Controller test. Summary (updated) - Added unit tests for fetching ResoruceUsage in both QoS Controller and Resource Estimator . Repository: mesos Description (updated) --- Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage from ResourceMonitor. This is the unit test for https://reviews.apache.org/r/34980/ and https://reviews.apache.org/r/35164/ Diffs (updated) - src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 Diff: https://reviews.apache.org/r/35157/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 34894: Add new message for Traffic Control statistics
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34894/#review86860 --- Ship it! LGTM. I'll wait for Ian's shipit. - Jie Yu On June 3, 2015, 9:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34894/ --- (Updated June 3, 2015, 9:54 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Add new message for Traffic Control statistics Diffs - include/mesos/mesos.proto 6186c92c6fe9cf8fa136d93d5af43e0377406baf Diff: https://reviews.apache.org/r/34894/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 35150: Add output stream operation for handle to use in port_mapping.cpp
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35150/#review86861 --- Ship it! src/linux/routing/handle.hpp https://reviews.apache.org/r/35150/#comment138976 2 lines apart. src/linux/routing/handle.hpp https://reviews.apache.org/r/35150/#comment138977 Ditto. - Jie Yu On June 5, 2015, 8:30 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35150/ --- (Updated June 5, 2015, 8:30 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Add output stream operation for handle to use in port_mapping.cpp Diffs - src/linux/routing/handle.hpp c107a7ef16f5060210b16b91e3a881af84ba6c10 Diff: https://reviews.apache.org/r/35150/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 35152: Document and consolidate qdisc handles
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/#review86867 --- Patch looks great! Reviews applied: [35150, 35152] All tests passed. - Mesos ReviewBot On June 5, 2015, 10:35 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35152/ --- (Updated June 5, 2015, 10:35 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2821 https://issues.apache.org/jira/browse/MESOS-2821 Repository: mesos Description --- Document and consolidate qdisc handles Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 Diff: https://reviews.apache.org/r/35152/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 35165: Extend fq_codel to allow parent and handle to be specified at runtime.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35165/#review86894 --- Patch looks great! Reviews applied: [35150, 35152, 35165] All tests passed. - Mesos ReviewBot On June 6, 2015, 12:02 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35165/ --- (Updated June 6, 2015, 12:02 a.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2332 https://issues.apache.org/jira/browse/MESOS-2332 Repository: mesos Description --- Extend fq_codel to allow parent and handle to be specified at runtime. Diffs - src/linux/routing/queueing/fq_codel.hpp 412af2d7da2c70324234ee75df75c71319b1365a src/linux/routing/queueing/fq_codel.cpp 3db5b9390bb5ae759eb1b7c9d1ac87cfca8d531e src/slave/containerizer/isolators/network/port_mapping.cpp 871e9cf1625d96d1feef50edd4081972c097d191 src/tests/routing_tests.cpp db5b5df48634ff322baf9328fc605b2667b56eed Diff: https://reviews.apache.org/r/35165/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 30774: Fetcher Cache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30774/#review86871 --- Just took a cursory glance since this is a huge diff, could it have been broken apart further? We've found large diffs like this one are next to impossible to review thoroughly :) src/slave/containerizer/fetcher.hpp https://reviews.apache.org/r/30774/#comment139000 We might be able to get away with more descriptive names here to avoid the need for these comments: ``` Bytes capacity; Bytes reserved; unsigned long fileCounter; ``` 'space' seems to suggest available space (to me), whereas 'capacity' seems pretty standard as a name for this. For 'tally', I can't tell from the name what is being tallied, but if we change the name to 'reserved' I have an understanding that this is the reserved space, not necessarily occupied but reserved for a purpose. src/slave/containerizer/fetcher.hpp https://reviews.apache.org/r/30774/#comment139052 Hard to tell why shared_ptr here is needed rather than Shared, or just Cache::Entry directly. Is there concurrent modification happening, or? src/slave/containerizer/fetcher.cpp https://reviews.apache.org/r/30774/#comment139051 No need for the stringify here and below. src/slave/containerizer/fetcher.cpp https://reviews.apache.org/r/30774/#comment139053 CHECK_LT will print the two numbers for you :) src/slave/containerizer/fetcher.cpp https://reviews.apache.org/r/30774/#comment139055 Seems like an odd message format, since normally a meaning follows from a ':' ``` Fetcher cache space overflow - space used: 2GB, exceeds total fetcher cache space: 1GB ``` Here's another format where the meaning is described after the colon: ``` Fetcher cache space overflow: 2GB used vs 1GB capacity ``` src/slave/containerizer/mesos/containerizer.hpp https://reviews.apache.org/r/30774/#comment139050 I can't tell why slave id is being passed here, is there something subtle going on? - Ben Mahler On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30774/ --- (Updated May 21, 2015, 4:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen. Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074 https://issues.apache.org/jira/browse/MESOS-2057 https://issues.apache.org/jira/browse/MESOS-2069 https://issues.apache.org/jira/browse/MESOS-2070 https://issues.apache.org/jira/browse/MESOS-2072 https://issues.apache.org/jira/browse/MESOS-2073 https://issues.apache.org/jira/browse/MESOS-2074 Repository: mesos Description --- Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features. Diffs - docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b docs/fetcher-cache-internals.md PRE-CREATION docs/fetcher.md PRE-CREATION include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e src/slave/containerizer/mesos/containerizer.cpp
Re: Review Request 34970: Cleaned up and generalized NoExecutorScheduler to be more configurable.
On June 5, 2015, 9:37 p.m., Vinod Kone wrote: src/examples/no_executor_framework.cpp, line 219 https://reviews.apache.org/r/34970/diff/2/?file=979734#file979734line219 LOG(FATAL)? because this is not possible with a command executor? Sure, this was to stay consistent with the other examples (e.g. persistent_volume_framework.cpp), but I'll change it. On June 5, 2015, 9:37 p.m., Vinod Kone wrote: src/examples/no_executor_framework.cpp, line 253 https://reviews.apache.org/r/34970/diff/2/?file=979734#file979734line253 s/active/launched/ ? This is all non-terminal (i.e. active) tasks. Launched seems to imply everything ever launched, no? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/#review86846 --- On June 5, 2015, 7:56 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34970/ --- (Updated June 5, 2015, 7:56 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Bugs: MESOS-2655 https://issues.apache.org/jira/browse/MESOS-2655 Repository: mesos Description --- Now the NoExecutorScheduler can take a custom command, as well as the resources for the task and how many tasks to run. This allows us to continue using it for an end-to-end test as part of make check, but also allows it to be used against a cluster in a long-lived fashion. Diffs - src/examples/no_executor_framework.cpp 1fb853d6e4a3deb3c36e6e661689697c8ebf898f src/tests/no_executor_framework_test.sh d2d395595b778bc543e1baaa0fd415dc622b647f src/tests/script.cpp 515e3141e1d517da4cfc1421d5301d0a3bd6ad51 Diff: https://reviews.apache.org/r/34970/diff/ Testing --- The existing no executor framework test picks this up. Since we do not have an non-zero estimator yet, I modified the slave to send revocable resources in order to manually test the revocable case. Thanks, Ben Mahler
Re: Review Request 34943: Added validation to flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/ --- (Updated June 5, 2015, 2:48 p.m.) Review request for mesos. Repository: mesos Description (updated) --- Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our loader and stringifier functors. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984 Diff: https://reviews.apache.org/r/34943/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 34943: Added validation to flags.
On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line514
I'll admit I'm still a bith hazy about the new { } initializer, but
could have this been:
```
char* argv[] = { (char*) /path/to/program,
(char*) --name1=billy joel };
```
Benjamin Hindman wrote:
I just copied this code. Let me give this a test and if so I'll cleanup
the others too.
This is definitely cleaner and we should do it this way going forward so I'm
going drop it here and follow up with a subsequent review that takes care of
this, thanks for pointing it out!
- Benjamin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
---
On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/
---
(Updated June 5, 2015, 2:48 p.m.)
Review request for mesos.
Repository: mesos
Description
---
Also refactored existing 'lambda::bind' arguments to use C++11 lambdas,
enabling us to get rid of our loader and stringifier functors.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/Makefile.am
6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp
87606d860dea3235ec70d127d3379065d42dc90b
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp
51d3ab020bd2bae1f12b66cac0da5383c813d5d7
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp
fda5ae1328a23a4371475652f921341dce7448d5
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
80450185f60c5b273face490e0bb9e695b0cb984
Diff: https://reviews.apache.org/r/34943/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33295/#review86800 --- Ship it! 3rdparty/libprocess/include/process/firewall.hpp https://reviews.apache.org/r/33295/#comment138884 First of all, sorry for leading you in the wrong direction here - but could you please start the description with a capital letter here and everywhere else as it has just decided and documented in our styleguides? 3rdparty/libprocess/include/process/firewall.hpp https://reviews.apache.org/r/33295/#comment138885 s/requiered/required/ 3rdparty/libprocess/include/process/firewall.hpp https://reviews.apache.org/r/33295/#comment138886 s/considering/considered/? 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/33295/#comment138887 I see that you copied this from the above but once you get rid of the duplication, could you also make sure that this comment really adds value (or gets killed)? 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/33295/#comment13 Let make this a std::move. 3rdparty/libprocess/src/tests/process_tests.cpp https://reviews.apache.org/r/33295/#comment138889 Could you move this comment three lines down before the actual request? 3rdparty/libprocess/src/tests/process_tests.cpp https://reviews.apache.org/r/33295/#comment138891 Could you please adjust the style of this comment towards those above? 3rdparty/libprocess/src/tests/process_tests.cpp https://reviews.apache.org/r/33295/#comment138893 s/a null pointer/an empty vector/ - Till Toenshoff On June 2, 2015, 9:57 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33295/ --- (Updated June 2, 2015, 9:57 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2620 https://issues.apache.org/jira/browse/MESOS-2620 Repository: mesos Description --- Introduces the interface `FirewallRule` which will be matched against incoming connections in order to allow them to be served or being blocked. For details, check the [design doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit). Diffs - 3rdparty/libprocess/include/Makefile.am 8aab0593f296c7aae71289f9bd6cf3eb3578a721 3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 3rdparty/libprocess/include/process/process.hpp 79d1719932a3fdc90b6247d3a77adee123e72435 3rdparty/libprocess/src/process.cpp e3de3cd6b536aaaf59784360aed546512dd04dc9 3rdparty/libprocess/src/tests/process_tests.cpp 67e582cc250a9767a389e2bd0cc68985477f3ffb Diff: https://reviews.apache.org/r/33295/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34943: Added validation to flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/ --- (Updated June 5, 2015, 2:48 p.m.) Review request for mesos. Changes --- Addressed Marco's comments. Also C++11 lambda'fied the remaining Flag functions currently implemented via functors. Repository: mesos Description --- See summary. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984 Diff: https://reviews.apache.org/r/34943/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 34944: Used flags validation to handle --help.
On June 2, 2015, 3:54 p.m., Marco Massenzio wrote: Sweet! Only comment is that this does not give the client of FlagsBase an option as to the behavior of --help (ie, it prints and exit: like it or lump it :). Which is perfectly fine, IMO, but was wondering whether this was too restrictive? (I haven't seen any other pattern in the 10+ refactorings I've done - so this should be just fine) Yeah, I'm not convinced this is the way we want to do this, more that it's an easy MVP with a flags validator (which motivated me to dust off my old flags validation patch). I'm going to discard this review for now while someone actually puts a little more thought into this. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34944/#review86231 --- On June 2, 2015, 2:46 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34944/ --- (Updated June 2, 2015, 2:46 p.m.) Review request for mesos and Marco Massenzio. Repository: mesos Description --- This is one possible way of handling processing --help. Also, this is missing a test. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 61a405f225d14acbc38a80d35570426cb05a3d0a Diff: https://reviews.apache.org/r/34944/diff/ Testing --- make check Thanks, Benjamin Hindman
