Re: Review Request 36910: Patch configure.ac to include $LIBS in the CRAM-MD5 check
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36910/#review93579 --- It's unclear what failed in that auto build. It appears unrelated (potentially). As a test I pulled my branch, and rebased from master, then configured a build and ran `make -j3 distcheck` and was successful in building. Can the auto check be re-run? - Chris Heller On July 29, 2015, 1:41 p.m., Chris Heller wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36910/ --- (Updated July 29, 2015, 1:41 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-3170 https://issues.apache.org/jira/browse/MESOS-3170 Repository: mesos Description --- [MESOS-3170] Add $LIBS to build path of the CRAM-MD5 test Diffs - b/configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 Diff: https://reviews.apache.org/r/36910/diff/ Testing --- See https://github.com/apache/mesos/pull/51 Verified build against statically linked OpenSSL 1.0.1e and Cyrus-SASL 2.1.26 Thanks, Chris Heller
Review Request 36951: MESOS-3052: optimize slave ID comparisons
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36951/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-3052 https://issues.apache.org/jira/browse/MESOS-3052 Repository: mesos Description --- MESOS-3052: optimize slave ID comparisons Diffs - src/master/allocator/mesos/hierarchical.hpp e278139f856888d6c6f538f7c0f664087e97f629 Diff: https://reviews.apache.org/r/36951/diff/ Testing --- Running in production for a few weeks, yields ~50% CPU reduction. Tested with make check and make bench. Thanks, James Peach
Re: Review Request 36951: MESOS-3052: optimize slave ID comparisons
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36951/#review93647 --- Patch looks great! Reviews applied: [36951] All tests passed. - Mesos ReviewBot On July 30, 2015, 8:43 p.m., James Peach wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36951/ --- (Updated July 30, 2015, 8:43 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3052 https://issues.apache.org/jira/browse/MESOS-3052 Repository: mesos Description --- MESOS-3052: optimize slave ID comparisons Diffs - src/master/allocator/mesos/hierarchical.hpp e278139f856888d6c6f538f7c0f664087e97f629 Diff: https://reviews.apache.org/r/36951/diff/ Testing --- Running in production for a few weeks, yields ~10% CPU reduction. Tested with make check and make bench. Thanks, James Peach
Re: Review Request 36929: Fixed a few issues in test launcher header.
On July 30, 2015, 4:45 p.m., Vinod Kone wrote:
src/tests/containerizer/launcher.hpp, lines 19-37
https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19
why did you remove these headers?
i think we decided to explicitly include all the headers that are used
in a file instead of depending on transitive includes.
Jie Yu wrote:
Is there a discussion somewhere? I think explicitly including all headers
make it hard to maintain (you'll need to adjust the header when the dependent
header changes). Also, it slows down the compilation.
Vinod Kone wrote:
This is the pattern we followed in the code base. I think it improves
readability and avoids gotchas. For example, it is not clear at all that the
headers you kept provide a string or a vector definition (What if those
headers don't #include string at a later time?). This is the relevant
blurb from the google style guide
```
You should include all the headers that define the symbols you rely upon
(except in cases of forward declaration). If you rely on symbols from bar.h,
don't count on the fact that you included foo.h which (currently) includes
bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its
intent to provide you the symbols of bar.h. However, any includes present in
the related header do not need to be included again in the related cc (i.e.,
foo.cc can rely on foo.h's includes).
```
Some relevant discussion:
http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
Jie Yu wrote:
I have to disagree. C++ is not like Java, we don't have an automated tool
to help you include all necessary headers (java does). As a result, that
makes maintaining the includes very tedious. Any time someone is doing a
refactor, he/she has to manually check all the includes to make sure it's
up-to-date. Unless we have an automated tool to help you include all
necessary headers, I think having less includes makes it easy to maintain.
The pattern I suggest here is:
```
base.hpp
#include string
class Base {
virtual void foo(const string str);
};
derived.hpp
#include base.hpp
class Derived : public Base {
virtual void foo(const string str);
};
```
We suggest don't include `string` in derived.hpp. This does not violate
google style as it states: unless foo.h explicitly demonstrates its intent
to provide you the symbols of bar.h. I think here, base.hpp clearly
demonstrates its intent to provide you the symbols for all its public
interfaces.
It's fine if we want do it that way, but lets do that after we have a
discussion on the dev list. It would be great to see if we can get some build
improvement numbers.
For now though I prefer we keep the codebase consistent. OK with you?
- Vinod
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
---
On July 30, 2015, 12:14 a.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/
---
(Updated July 30, 2015, 12:14 a.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
Fixed a few issues in test launcher header.
Diffs
-
src/tests/containerizer/launcher.hpp
b80e84196f8b494e6e5a3c96c86f63fe432a7bb0
Diff: https://reviews.apache.org/r/36929/diff/
Testing
---
make check
Thanks,
Jie Yu
Re: Review Request 36929: Fixed a few issues in test launcher header.
Jie, I thought that duplicate includes of headers don't have a significant
impact on compile times given our include guards, why do you say it slows
down the compilation?
e.g. https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html
On Thu, Jul 30, 2015 at 12:57 PM, Vinod Kone vinodk...@gmail.com wrote:
On July 30, 2015, 4:45 p.m., Vinod Kone wrote:
src/tests/containerizer/launcher.hpp, lines 19-37
https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19
why did you remove these headers?
i think we decided to explicitly include all the headers that are
used in a file instead of depending on transitive includes.
Jie Yu wrote:
Is there a discussion somewhere? I think explicitly including all
headers make it hard to maintain (you'll need to adjust the header when the
dependent header changes). Also, it slows down the compilation.
Vinod Kone wrote:
This is the pattern we followed in the code base. I think it
improves readability and avoids gotchas. For example, it is not clear at
all that the headers you kept provide a string or a vector definition (What
if those headers don't #include string at a later time?). This is the
relevant blurb from the google style guide
```
You should include all the headers that define the symbols you rely
upon (except in cases of forward declaration). If you rely on symbols from
bar.h, don't count on the fact that you included foo.h which (currently)
includes bar.h: include bar.h yourself, unless foo.h explicitly
demonstrates its intent to provide you the symbols of bar.h. However, any
includes present in the related header do not need to be included again in
the related cc (i.e., foo.cc can rely on foo.h's includes).
```
Some relevant discussion:
http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
Jie Yu wrote:
I have to disagree. C++ is not like Java, we don't have an automated
tool to help you include all necessary headers (java does). As a result,
that makes maintaining the includes very tedious. Any time someone is doing
a refactor, he/she has to manually check all the includes to make sure it's
up-to-date. Unless we have an automated tool to help you include all
necessary headers, I think having less includes makes it easy to maintain.
The pattern I suggest here is:
```
base.hpp
#include string
class Base {
virtual void foo(const string str);
};
derived.hpp
#include base.hpp
class Derived : public Base {
virtual void foo(const string str);
};
```
We suggest don't include `string` in derived.hpp. This does not
violate google style as it states: unless foo.h explicitly demonstrates
its intent to provide you the symbols of bar.h. I think here, base.hpp
clearly demonstrates its intent to provide you the symbols for all its
public interfaces.
It's fine if we want do it that way, but lets do that after we have a
discussion on the dev list. It would be great to see if we can get some
build improvement numbers.
For now though I prefer we keep the codebase consistent. OK with you?
- Vinod
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
---
On July 30, 2015, 12:14 a.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/
---
(Updated July 30, 2015, 12:14 a.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
Fixed a few issues in test launcher header.
Diffs
-
src/tests/containerizer/launcher.hpp
b80e84196f8b494e6e5a3c96c86f63fe432a7bb0
Diff: https://reviews.apache.org/r/36929/diff/
Testing
---
make check
Thanks,
Jie Yu
Re: Review Request 36951: MESOS-3052: optimize slave ID comparisons
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36951/ --- (Updated July 30, 2015, 8:43 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3052 https://issues.apache.org/jira/browse/MESOS-3052 Repository: mesos Description --- MESOS-3052: optimize slave ID comparisons Diffs - src/master/allocator/mesos/hierarchical.hpp e278139f856888d6c6f538f7c0f664087e97f629 Diff: https://reviews.apache.org/r/36951/diff/ Testing (updated) --- Running in production for a few weeks, yields ~10% CPU reduction. Tested with make check and make bench. Thanks, James Peach
Re: Review Request 36929: Fixed a few issues in test launcher header.
aha, I just thought it might slow down the compilation. But looks like it
will not given the optimization. I guess clang should have the same
optimization as well.
The burden of updating the headers while doing refactor is real. It'll be
really cool if we can automate this.
BTW: the code base is already not consistent. For example:
https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/filesystem/posix.hpp
We don't include future.hpp, option.hpp, etc.
- Jie
On Thu, Jul 30, 2015 at 1:08 PM, Benjamin Mahler benjamin.mah...@gmail.com
wrote:
Jie, I thought that duplicate includes of headers don't have a significant
impact on compile times given our include guards, why do you say it slows
down the compilation?
e.g. https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html
On Thu, Jul 30, 2015 at 12:57 PM, Vinod Kone vinodk...@gmail.com wrote:
On July 30, 2015, 4:45 p.m., Vinod Kone wrote:
src/tests/containerizer/launcher.hpp, lines 19-37
https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19
why did you remove these headers?
i think we decided to explicitly include all the headers that are
used in a file instead of depending on transitive includes.
Jie Yu wrote:
Is there a discussion somewhere? I think explicitly including all
headers make it hard to maintain (you'll need to adjust the header when the
dependent header changes). Also, it slows down the compilation.
Vinod Kone wrote:
This is the pattern we followed in the code base. I think it
improves readability and avoids gotchas. For example, it is not clear at
all that the headers you kept provide a string or a vector definition (What
if those headers don't #include string at a later time?). This is the
relevant blurb from the google style guide
```
You should include all the headers that define the symbols you rely
upon (except in cases of forward declaration). If you rely on symbols from
bar.h, don't count on the fact that you included foo.h which (currently)
includes bar.h: include bar.h yourself, unless foo.h explicitly
demonstrates its intent to provide you the symbols of bar.h. However, any
includes present in the related header do not need to be included again in
the related cc (i.e., foo.cc can rely on foo.h's includes).
```
Some relevant discussion:
http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
Jie Yu wrote:
I have to disagree. C++ is not like Java, we don't have an
automated tool to help you include all necessary headers (java does). As a
result, that makes maintaining the includes very tedious. Any time someone
is doing a refactor, he/she has to manually check all the includes to make
sure it's up-to-date. Unless we have an automated tool to help you include
all necessary headers, I think having less includes makes it easy to
maintain.
The pattern I suggest here is:
```
base.hpp
#include string
class Base {
virtual void foo(const string str);
};
derived.hpp
#include base.hpp
class Derived : public Base {
virtual void foo(const string str);
};
```
We suggest don't include `string` in derived.hpp. This does not
violate google style as it states: unless foo.h explicitly demonstrates
its intent to provide you the symbols of bar.h. I think here, base.hpp
clearly demonstrates its intent to provide you the symbols for all its
public interfaces.
It's fine if we want do it that way, but lets do that after we have a
discussion on the dev list. It would be great to see if we can get some
build improvement numbers.
For now though I prefer we keep the codebase consistent. OK with you?
- Vinod
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
---
On July 30, 2015, 12:14 a.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/
---
(Updated July 30, 2015, 12:14 a.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
Fixed a few issues in test launcher header.
Diffs
-
src/tests/containerizer/launcher.hpp
b80e84196f8b494e6e5a3c96c86f63fe432a7bb0
Diff: https://reviews.apache.org/r/36929/diff/
Testing
---
make check
Thanks,
Jie Yu
Review Request 36958: Fixed a bug in authentication validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- When --flags.authenticate_frameworks is false, we still should be checking that the authenticated principal matches the framework's principal. Diffs - src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 Diff: https://reviews.apache.org/r/36958/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36927/#review93657 --- Ship it! Ship It! - Vinod Kone On July 30, 2015, 10:16 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36927/ --- (Updated July 30, 2015, 10:16 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Framework authorization is now done through `authorizeFramework` which is consistent with `authorizeTask`. Also, `validateFrameworkAuthentication` captures the validation for authentication. I also made registration and re-registration consistent: * Both perform the check for root submission, rather than just registration. * Authentication checks in `_registerFramework` and `_reregisterFramework` are now comprehensive (thanks to `validateFrameworkAuthentication`). Diffs - src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 Diff: https://reviews.apache.org/r/36927/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36958: Fixed a bug in authentication validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/#review93679 --- Ship it! src/tests/authentication_tests.cpp (line 199) https://reviews.apache.org/r/36958/#comment148095 s/,/ than Credential::principal/ ? s/when/even when/ ? do we already have a test for when authentication is required? if not, would be nice to have one. maybe parameterize this test? - Vinod Kone On July 30, 2015, 11:54 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/ --- (Updated July 30, 2015, 11:54 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- When --flags.authenticate_frameworks is false, we still should be checking that the authenticated principal matches the framework's principal. Diffs - src/master/master.cpp ce27dbe58d0afc2363991695e7af212616e1f09a src/tests/authentication_tests.cpp 5126ed4acbdf4c8508133bed35f98b0da3cca11e Diff: https://reviews.apache.org/r/36958/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 34129: Add 2 optional args advertise_ip and advertise_port for libprocess to advertise.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34129/#review93678 --- Patch looks great! Reviews applied: [34128, 34129] All tests passed. - Mesos ReviewBot On July 30, 2015, 10:36 p.m., Anindya Sinha wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34129/ --- (Updated July 30, 2015, 10:36 p.m.) Review request for mesos and Cosmin Lehene. Bugs: MESOS-809 https://issues.apache.org/jira/browse/MESOS-809 Repository: mesos Description --- If set, these IP/Port shall be advertised by libprocess (although bind is not done on this IP/Port). If not set, libprocess advertises the IP/Port on which bind was done. Command line arguments added: advertise_ip: IP address advertised to reach mesos master. advertise_port: Port advertised to reach mesos master (used with advertise_ip). Diffs - docs/configuration.md 506bab15a452f8b1b22328d0c5e9c46af6dfebff docs/operational-guide.md ef81db672bd5c8fab172336956ab03461ae35064 src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce Diff: https://reviews.apache.org/r/34129/diff/ Testing --- Testing: make test Thanks, Anindya Sinha
Re: Review Request 36828: Used std::thread instead of pthread for stout proc tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36828/#review93654 --- Ship it! I'll fix up and commit, thanks! 3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp (line 107) https://reviews.apache.org/r/36828/#comment148067 s//mutex, cond, stop/ - Benjamin Hindman On July 27, 2015, 8:45 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36828/ --- (Updated July 27, 2015, 8:45 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-3118 https://issues.apache.org/jira/browse/MESOS-3118 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp 5d24f21f63433b8525370736dd630880d324ebeb Diff: https://reviews.apache.org/r/36828/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34128: Enable different IP/Port for external access.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34128/ --- (Updated July 30, 2015, 10:36 p.m.) Review request for mesos. Changes --- Changed environment variables to LIBPROCESS_ADVERTISE_IP and LIBPROCESS_ADVERTISE_PORT. Bugs: MESOS-809 https://issues.apache.org/jira/browse/MESOS-809 Repository: mesos Description (updated) --- Expose environment variables LIBPROCESS_ADVERTISE_IP and LIBPROCESS_ADVERTISE_PORT as the IP and port which libprocess would advertise (if set). If not set, it defaults to the IP and port on which it binded to. Diffs (updated) - 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c Diff: https://reviews.apache.org/r/34128/diff/ Testing --- Testing: make test Thanks, Anindya Sinha
Re: Review Request 34129: Add 2 optional args advertise_ip and advertise_port for libprocess to advertise.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34129/ --- (Updated July 30, 2015, 10:36 p.m.) Review request for mesos and Cosmin Lehene. Changes --- Changed command line args to advertise_ip and advertise_port, and environment variables to LIBPROCESS_ADVERTISE_IP and LIBPROCESS_ADVERTISE_PORT. Summary (updated) - Add 2 optional args advertise_ip and advertise_port for libprocess to advertise. Bugs: MESOS-809 https://issues.apache.org/jira/browse/MESOS-809 Repository: mesos Description (updated) --- If set, these IP/Port shall be advertised by libprocess (although bind is not done on this IP/Port). If not set, libprocess advertises the IP/Port on which bind was done. Command line arguments added: advertise_ip: IP address advertised to reach mesos master. advertise_port: Port advertised to reach mesos master (used with advertise_ip). Diffs (updated) - docs/configuration.md 506bab15a452f8b1b22328d0c5e9c46af6dfebff docs/operational-guide.md ef81db672bd5c8fab172336956ab03461ae35064 src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce Diff: https://reviews.apache.org/r/34129/diff/ Testing --- Testing: make test Thanks, Anindya Sinha
Re: Review Request 36844: Libprocess: Used THREAD_LOCAL to replace ThreadLocal.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36844/#review93659 --- Ship it! Ship It! - Benjamin Hindman On July 27, 2015, 9:15 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36844/ --- (Updated July 27, 2015, 9:15 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-3119 https://issues.apache.org/jira/browse/MESOS-3119 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/Makefile.am bd95fe197532131255072a86aba83cd5e822419a 3rdparty/libprocess/include/process/executor.hpp 434d23ac8119e24454f8bc8324d7b26ec62a85c7 3rdparty/libprocess/include/process/process.hpp 8620547148f8a69d5b661eaf08063ca72347b6a4 3rdparty/libprocess/src/libev.hpp fd26728fe9c0688b82fd75f0bfc83b8d0a3b8581 3rdparty/libprocess/src/libev.cpp 55ed6efd47ac1864af828e68320c1929843cde57 3rdparty/libprocess/src/libevent.hpp 3a0a46ba95db95f4a007ec755012ac969d1c6cd9 3rdparty/libprocess/src/libevent.cpp 1f175a4ae83391152d064779c6ab69d31cbaf867 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c Diff: https://reviews.apache.org/r/36844/diff/ Testing --- make check with libev and libevent. Requires validation on OSX. Thanks, Joris Van Remoortere
Re: Review Request 36958: Fixed a bug in authentication validation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36958/#review93658
---
Mind adding a test for this if one doesn't exist already? Should be
straightforward.
src/master/master.cpp (lines 1565 - 1573)
https://reviews.apache.org/r/36958/#comment148069
why the nested if loop?
```
if (frameworkInfo.has_principal()
authenticated.contains(from)
fraemworkInfo.principal() != authenticated[from]) {
}
```
src/master/master.cpp (line 1567)
https://reviews.apache.org/r/36958/#comment148070
not yours, but
s/framweworks/frameworks/
- Vinod Kone
On July 30, 2015, 10:16 p.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36958/
---
(Updated July 30, 2015, 10:16 p.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
When --flags.authenticate_frameworks is false, we still should be
checking that the authenticated principal matches the framework's
principal.
Diffs
-
src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32
Diff: https://reviews.apache.org/r/36958/diff/
Testing
---
make check
Thanks,
Ben Mahler
Re: Review Request 36865: Style change: Space after the ... in variadic templates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36865/#review93669 --- Ship it! Ship It! - Joris Van Remoortere On July 29, 2015, 11:51 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36865/ --- (Updated July 29, 2015, 11:51 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Repository: mesos Description --- There are a few instances of `typename...` and `typename ...` in the code base. This patch normalizes those to just `typename...` . Diffs - 3rdparty/libprocess/include/process/help.hpp 316ed21b5300496f283d0c331fa6d9dad441d332 Diff: https://reviews.apache.org/r/36865/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 36929: Fixed a few issues in test launcher header.
On July 30, 2015, 9:45 a.m., Vinod Kone wrote:
src/tests/containerizer/launcher.hpp, lines 19-37
https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19
why did you remove these headers?
i think we decided to explicitly include all the headers that are used
in a file instead of depending on transitive includes.
Jie Yu wrote:
Is there a discussion somewhere? I think explicitly including all headers
make it hard to maintain (you'll need to adjust the header when the dependent
header changes). Also, it slows down the compilation.
Vinod Kone wrote:
This is the pattern we followed in the code base. I think it improves
readability and avoids gotchas. For example, it is not clear at all that the
headers you kept provide a string or a vector definition (What if those
headers don't #include string at a later time?). This is the relevant
blurb from the google style guide
```
You should include all the headers that define the symbols you rely upon
(except in cases of forward declaration). If you rely on symbols from bar.h,
don't count on the fact that you included foo.h which (currently) includes
bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its
intent to provide you the symbols of bar.h. However, any includes present in
the related header do not need to be included again in the related cc (i.e.,
foo.cc can rely on foo.h's includes).
```
Some relevant discussion:
http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
Jie Yu wrote:
I have to disagree. C++ is not like Java, we don't have an automated tool
to help you include all necessary headers (java does). As a result, that
makes maintaining the includes very tedious. Any time someone is doing a
refactor, he/she has to manually check all the includes to make sure it's
up-to-date. Unless we have an automated tool to help you include all
necessary headers, I think having less includes makes it easy to maintain.
The pattern I suggest here is:
```
base.hpp
#include string
class Base {
virtual void foo(const string str);
};
derived.hpp
#include base.hpp
class Derived : public Base {
virtual void foo(const string str);
};
```
We suggest don't include `string` in derived.hpp. This does not violate
google style as it states: unless foo.h explicitly demonstrates its intent
to provide you the symbols of bar.h. I think here, base.hpp clearly
demonstrates its intent to provide you the symbols for all its public
interfaces.
Vinod Kone wrote:
It's fine if we want do it that way, but lets do that after we have a
discussion on the dev list. It would be great to see if we can get some build
improvement numbers.
For now though I prefer we keep the codebase consistent. OK with you?
FWIW here is what include-what-you-use interprets what consititutes an
intent: https://code.google.com/p/include-what-you-use/wiki/WhatIsAUse, which
is in line with Jie's interpretation: if the dependency uses something on the
interface it's considered exporting the symbols whereas using it in its
implementation is not.
+1 for seeking agreement from dev@.
- Jiang Yan
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
---
On July 29, 2015, 5:14 p.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/
---
(Updated July 29, 2015, 5:14 p.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
Fixed a few issues in test launcher header.
Diffs
-
src/tests/containerizer/launcher.hpp
b80e84196f8b494e6e5a3c96c86f63fe432a7bb0
Diff: https://reviews.apache.org/r/36929/diff/
Testing
---
make check
Thanks,
Jie Yu
Re: Review Request 36864: Style change: Space after the ... in variadic templates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36864/#review93667 --- Ship it! Ship It! 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp (line 50) https://reviews.apache.org/r/36864/#comment148078 Accidentally removed an empty line. - Joris Van Remoortere On July 29, 2015, 11:51 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36864/ --- (Updated July 29, 2015, 11:51 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Repository: mesos Description --- There are a few instances of `typename...` and `typename ...` in the code base. This patch normalizes those to just `typename...` . Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 4e8c3bd1e9abf0ff24f78c8385ed9625719dcf8c 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 81f6e50baca95ad7a3a0ac1434c8db1c4de6adf2 Diff: https://reviews.apache.org/r/36864/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 36958: Fixed a bug in authentication validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/#review93668 --- Bad patch! Reviews applied: [36927] Failed command: ./support/apply-review.sh -n -r 36927 Error: 2015-07-30 23:07:34 URL:https://reviews.apache.org/r/36927/diff/raw/ [19346/19346] - 36927.patch [1] error: patch failed: src/master/master.hpp:575 error: src/master/master.hpp: patch does not apply error: patch failed: src/master/master.cpp:1524 error: src/master/master.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On July 30, 2015, 10:16 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/ --- (Updated July 30, 2015, 10:16 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- When --flags.authenticate_frameworks is false, we still should be checking that the authenticated principal matches the framework's principal. Diffs - src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 Diff: https://reviews.apache.org/r/36958/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36783: Windows: Header splitting continued (stout/os.hpp)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36783/#review93672 --- Ship it! Ship It! - Benjamin Hindman On July 29, 2015, 11:24 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36783/ --- (Updated July 29, 2015, 11:24 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Alex Clemmer, and Joris Van Remoortere. Bugs: MESOS-3102 https://issues.apache.org/jira/browse/MESOS-3102 Repository: mesos Description --- MESOS-3102: Stout library header splitting, to support the Windows Containerizer. Splits apart os.hpp only. See the prior review in the chain for the pattern. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 5c19e3ef8ba50ab007eda26b752441f076ca7ed0 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 818560f8ce20126e0aa4af6ce368c973c9616c74 3rdparty/libprocess/3rdparty/stout/include/stout/os/os.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/windows/preprocessor.hpp PRE-CREATION Diff: https://reviews.apache.org/r/36783/diff/ Testing --- `make` and `make check` (Mac OSX). Build with MSVC Enterprise 2015 [thanks to Alex (hausdorff)]. Thanks, Joseph Wu
Re: Review Request 36958: Fixed a bug in authentication validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/#review93687 --- Bad patch! Reviews applied: [36927] Failed command: ./support/apply-review.sh -n -r 36927 Error: 2015-07-31 00:49:14 URL:https://reviews.apache.org/r/36927/diff/raw/ [19346/19346] - 36927.patch [1] error: patch failed: src/master/master.hpp:575 error: src/master/master.hpp: patch does not apply error: patch failed: src/master/master.cpp:1524 error: src/master/master.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On July 30, 2015, 11:54 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/ --- (Updated July 30, 2015, 11:54 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- When --flags.authenticate_frameworks is false, we still should be checking that the authenticated principal matches the framework's principal. Diffs - src/master/master.cpp ce27dbe58d0afc2363991695e7af212616e1f09a src/tests/authentication_tests.cpp 5126ed4acbdf4c8508133bed35f98b0da3cca11e Diff: https://reviews.apache.org/r/36958/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 36956: Created a test abstraction for preparing test rootfs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36956/ --- Review request for mesos, Ian Downes, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-3179 https://issues.apache.org/jira/browse/MESOS-3179 Repository: mesos Description --- Created a test abstraction for preparing test rootfs. Diffs - src/tests/containerizer/launch_tests.cpp 73c8c5fa17936b1bab4817e9f1e691144e87c35f src/tests/containerizer/rootfs.hpp PRE-CREATION Diff: https://reviews.apache.org/r/36956/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36959: Code safety: Remove capture by reference of a temporary.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36959/#review93663 --- Ship it! Ship It! - Joris Van Remoortere On July 30, 2015, 10:46 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36959/ --- (Updated July 30, 2015, 10:46 p.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Repository: mesos Description --- For more detail on why this is bad, see MESOS-2629. Diffs - src/master/registrar.hpp 6bc78c4904effde8e4108cf226467419dd014e2f Diff: https://reviews.apache.org/r/36959/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 36930: Forced the network isolator to use the mount namespace.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36930/#review93680 --- Ship it! Could you make a comment that port mapping doesn't need mount namespace itself, i.e., the make-share outside and the make-slave inside on /var/run/netns are noops when it's disabled, but doing so avoids the race in MESOS-1558 when it is enabled, in case when it is required by other components of mesos? - Chi Zhang On July 30, 2015, 12:19 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36930/ --- (Updated July 30, 2015, 12:19 a.m.) Review request for mesos, Chi Zhang and Vinod Kone. Repository: mesos Description --- Forced the network isolator to use the mount namespace. The code of the network isolator actually relies on the fact that the child is in a seprate mount namespace. For example: https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/network/port_mapping.cpp#L1527 https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/network/port_mapping.cpp#L3533 It originally depends on mount namespace, but was removed in this patch: https://reviews.apache.org/r/26274 That was a bug to me. It didn't cause any issue because we don't clone the mounts (since we are not using mount namespace) anymore after the above patch. So the kernel won't have an extra reference to the mount when we try to umount it in `_cleanup()`. Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 3f6e9df8711995d0dd3903c6170fdd5ad61aac5a Diff: https://reviews.apache.org/r/36930/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36958: Fixed a bug in authentication validation.
On July 31, 2015, 12:05 a.m., Vinod Kone wrote: src/tests/authentication_tests.cpp, line 199 https://reviews.apache.org/r/36958/diff/2/?file=1025323#file1025323line199 s/,/ than Credential::principal/ ? s/when/even when/ ? do we already have a test for when authentication is required? if not, would be nice to have one. maybe parameterize this test? Yeah, we already have a test, but not one for when authentication is not required. Looks like many of the AuthenticationTests can be parameterized, likely also need AuthenticationRequiredTest and AuthenticationNotRequiredTest to test unauthenticated frameworks. Will punt for now. Seems like we should change the phrasing here to say authentication required / not required rather than enabled / disabled? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/#review93679 --- On July 30, 2015, 11:54 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36958/ --- (Updated July 30, 2015, 11:54 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- When --flags.authenticate_frameworks is false, we still should be checking that the authenticated principal matches the framework's principal. Diffs - src/master/master.cpp ce27dbe58d0afc2363991695e7af212616e1f09a src/tests/authentication_tests.cpp 5126ed4acbdf4c8508133bed35f98b0da3cca11e Diff: https://reviews.apache.org/r/36958/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36946: Factored out the pattern for URL generation in a fetcher test.
On July 30, 2015, 5:14 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 332 https://reviews.apache.org/r/36946/diff/1/?file=1025117#file1025117line332 ``` // Tests whether fetcher can process URIs that contain leading whitespace ``` Does this patch broke this? Nop, it did NOT include the UT for that case (Tests whether fetcher can process URIs that contain leading whitespace). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/#review93605 --- On July 30, 2015, 4:37 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/ --- (Updated July 30, 2015, 4:37 p.m.) Review request for mesos, Bernd Mathiske and Klaus Ma. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Factored out the pattern for URL generation in (another)fetcher test. Diffs - src/tests/fetcher_tests.cpp 3ded3c037bdfe7095aa15503d81a8d2ee9d420df Diff: https://reviews.apache.org/r/36946/diff/ Testing --- GTEST_FILTER='FetcherTest.OSNetUriSpaceTest' make check Thanks, Artem Harutyunyan
Re: Review Request 36821: Fix disable endpoints rule fails to recognize HTTP path delegates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/#review93557 --- Ship it! - Alexander Rojas On July 29, 2015, 7:40 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/ --- (Updated July 29, 2015, 7:40 p.m.) Review request for mesos, Adam B and Alexander Rojas. Bugs: MESOS-3143 https://issues.apache.org/jira/browse/MESOS-3143 Repository: mesos Description --- Fix disable endpoints rule fails to recognize HTTP path delegates. Diffs - 3rdparty/libprocess/Makefile.am e9c42d7ecd44eebeadcc3d1d5011e01674b3415a 3rdparty/libprocess/include/process/firewall.hpp f92a6c5bbd4af0cab43e487b0c347c30059cf162 3rdparty/libprocess/include/process/process.hpp 8620547148f8a69d5b661eaf08063ca72347b6a4 3rdparty/libprocess/src/firewall.cpp PRE-CREATION 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c Diff: https://reviews.apache.org/r/36821/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 36663: Added ip_address field to MasterInfo
On July 29, 2015, 1:01 p.m., Alexander Rukletsov wrote: include/mesos/mesos.proto, line 399 https://reviews.apache.org/r/36663/diff/5/?file=1021578#file1021578line399 `ip` and `port` are required, while `address` is optional. Is it intentional / doesn't it introduce a pitfall? Marco Massenzio wrote: You cannot add a `required` field in an existing protobuf, or you break existing servers (in particular, we would have all =0.22.x Mesos to break a 0.23 when the latter does Leader contention/detection) - that's PB 101 :) Did you mean the fields **inside** `Address` are required, or the **siblings** of `address`? If the latter, there's nothing you can do about it - as I mentioned, new fields **Must** be `optional` and you can't mess with existing ones. If the former, no - it's perfectly logically consistent: `address` may or may not be there; if it is there, then it **must** have `ip` and `port` set. Sorry for not expressing myself clearly. 1. IIUC, we can introduce `required` fields in the same way we retire them: via optional transisiton. I am not saying we definitely need to do that here (I haven't though about that long enough), but it looks technically possible to me. 2. If we forget about technical difficulties for a while, it looks to me that we change a sematics: what was required becomes optional. Does it make sense to write a comment saying that the optional `address` field is guaranteed to be there? Does it make sense to introduce some sort of validation that this field is always present even though it's optional? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36663/#review93441 --- On July 25, 2015, 12:36 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36663/ --- (Updated July 25, 2015, 12:36 a.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Bugs: MESOS-2736 https://issues.apache.org/jira/browse/MESOS-2736 Repository: mesos Description --- Added address field to MasterInfo As part of the new HTTP API and the need to provide a better interface for clients that do not integrate libmesos, we provide the IP address of the Leader Master in the information that gets serialized (in JSON) to ZooKeeper. This will eventually supersede the `ip`, `port` and `hostname` fields that are currently in MasterInfo an which cannot fully support IPv6 addresses. Jira: MESOS-2736 Review: https://reviews.apache.org/r/36663 Diffs - CHANGELOG 1a8649d00f3bcc792a75c11dedeb42a667e2ce88 include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a src/common/protobuf_utils.cpp d900707ae64ad92c0c0ddc2996324b61121c8594 src/master/master.cpp 7796630a93705bd62157e98e1e4855c68ea7cd0a src/tests/master_contender_detector_tests.cpp d7a3b46b2e437818631064ae34317e49c9aa3748 Diff: https://reviews.apache.org/r/36663/diff/ Testing --- make check (also tested via [zk-mesos](https://github.com/massenz/zk-mesos) that the information serialized to ZK is readable and as expected). Also ran 2x 0.23 master builds against a 0.24 one with this patch applied; getting both versions in turn to be Leader, and also ran a 0.23 Slave with a 0.24 Leader, and they all recognized each other. Thanks, Marco Massenzio
Re: Review Request 36197: Documented how to become a committer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/#review93558 --- Ship it! Ship It! - Adam B On July 28, 2015, 11:02 a.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/ --- (Updated July 28, 2015, 11:02 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, and Vinod Kone. Bugs: MESOS-1815 https://issues.apache.org/jira/browse/MESOS-1815 Repository: mesos Description --- Added new document committer-candidate-checklist.md and wrote a paragraph about the path to committership in committers.md. Diffs - docs/committer-candidate-checklist.md PRE-CREATION docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 Diff: https://reviews.apache.org/r/36197/diff/ Testing --- The rendered files can be viewed here: https://gist.github.com/bernd-mesos/00de63ae13efec4331be Thanks, Bernd Mathiske
Review Request 36946: Factored out the pattern for URL generation in a fetcher test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/ --- Review request for mesos, Bernd Mathiske and Klaus Ma. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Factored out the pattern for URL generation in (another)fetcher test. Diffs - src/tests/fetcher_tests.cpp 3ded3c037bdfe7095aa15503d81a8d2ee9d420df Diff: https://reviews.apache.org/r/36946/diff/ Testing --- GTEST_FILTER='FetcherTest.OSNetUriSpaceTest' make check Thanks, Artem Harutyunyan
Re: Review Request 36910: Patch configure.ac to include $LIBS in the CRAM-MD5 check
On July 30, 2015, 12:28 p.m., Chris Heller wrote: It's unclear what failed in that auto build. It appears unrelated (potentially). As a test I pulled my branch, and rebased from master, then configured a build and ran `make -j3 distcheck` and was successful in building. Can the auto check be re-run? nothing wrong with your patch. there was another problem on the master branch that we pushed a fix for. if you can rebase off the latest master and update this review, the bot will run again. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36910/#review93579 --- On July 29, 2015, 1:41 p.m., Chris Heller wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36910/ --- (Updated July 29, 2015, 1:41 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-3170 https://issues.apache.org/jira/browse/MESOS-3170 Repository: mesos Description --- [MESOS-3170] Add $LIBS to build path of the CRAM-MD5 test Diffs - b/configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 Diff: https://reviews.apache.org/r/36910/diff/ Testing --- See https://github.com/apache/mesos/pull/51 Verified build against statically linked OpenSSL 1.0.1e and Cyrus-SASL 2.1.26 Thanks, Chris Heller
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32587/ --- (Updated July 30, 2015, 12:54 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- removed trailing space. Bugs: MESOS-2559 https://issues.apache.org/jira/browse/MESOS-2559 Repository: mesos Description --- FrameworkID can be retrieved from RunTaskMessage.framework. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs (updated) - src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510 src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea Diff: https://reviews.apache.org/r/32587/diff/ Testing --- make check Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks. Thanks, Kapil Arya
Re: Review Request 36947: Fix new/delete mismatch in stout test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36947/#review93619 --- Ship it! Note that subprocess is in libprocess rather than stout :) - Ben Mahler On July 30, 2015, 6:09 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36947/ --- (Updated July 30, 2015, 6:09 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Jie Yu, and Vinod Kone. Bugs: MESOS-3175 https://issues.apache.org/jira/browse/MESOS-3175 Repository: mesos Description --- Fix new/delete mismatch in stout test. Diffs - 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36947/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36947: Fix new/delete mismatch in stout test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36947/#review93622 --- Patch looks great! Reviews applied: [36947] All tests passed. - Mesos ReviewBot On July 30, 2015, 6:09 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36947/ --- (Updated July 30, 2015, 6:09 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Jie Yu, and Vinod Kone. Bugs: MESOS-3175 https://issues.apache.org/jira/browse/MESOS-3175 Repository: mesos Description --- Fix new/delete mismatch in stout test. Diffs - 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36947/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36819: Use setup.py in python cli package.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36819/#review93552 --- Patch looks great! Reviews applied: [36819] All tests passed. - Mesos ReviewBot On July 30, 2015, 2:49 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36819/ --- (Updated July 30, 2015, 2:49 a.m.) Review request for mesos, Benjamin Hindman and Marco Massenzio. Bugs: MESOS-3149 https://issues.apache.org/jira/browse/MESOS-3149 Repository: mesos Description --- Use setup.py in python cli package. Diffs - Makefile.am f8e958d376efa55ace7c5727a31b8e747403641e bin/mesos.sh.in 5cbeac4330a9f45fc6d54b8c2d383f48e4098f95 configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec src/cli/python/mesos/__init__.py 028b0d27fb193bac96f2a6a3201ee4cc8fd369ef src/cli/python/mesos/cli.py src/cli/python/mesos/futures.py src/cli/python/mesos/http.py src/python/cli/src/mesos/__init__.py PRE-CREATION src/python/interface/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 src/python/native/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 src/python/protocol/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 Diff: https://reviews.apache.org/r/36819/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 36929: Fixed a few issues in test launcher header.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/#review93599 --- src/tests/containerizer/launcher.hpp (lines 19 - 24) https://reviews.apache.org/r/36929/#comment147982 why did you remove these headers? i think we decided to explicitly include all the headers that are used in a file instead of depending on transitive includes. - Vinod Kone On July 30, 2015, 12:14 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/ --- (Updated July 30, 2015, 12:14 a.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Fixed a few issues in test launcher header. Diffs - src/tests/containerizer/launcher.hpp b80e84196f8b494e6e5a3c96c86f63fe432a7bb0 Diff: https://reviews.apache.org/r/36929/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32587/ --- (Updated July 30, 2015, 12:18 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- Addressed Adam's comments. Bugs: MESOS-2559 https://issues.apache.org/jira/browse/MESOS-2559 Repository: mesos Description --- FrameworkID can be retrieved from RunTaskMessage.framework. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs (updated) - src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510 src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea Diff: https://reviews.apache.org/r/32587/diff/ Testing --- make check Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks. Thanks, Kapil Arya
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
On July 30, 2015, 5:03 a.m., Adam B wrote: src/slave/slave.cpp, line 1240 https://reviews.apache.org/r/32587/diff/5/?file=1023551#file1023551line1240 Maybe not a CHECK, since that would kill the slave. How about just logging an error and, if you're feeling generous, maybe sending back TASK_LOST? A TASK_LOST message would require a framework-id, so just ignoring the runTask message and logging an error instead. - Kapil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32587/#review93560 --- On July 30, 2015, 12:18 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32587/ --- (Updated July 30, 2015, 12:18 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2559 https://issues.apache.org/jira/browse/MESOS-2559 Repository: mesos Description --- FrameworkID can be retrieved from RunTaskMessage.framework. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs - src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510 src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea Diff: https://reviews.apache.org/r/32587/diff/ Testing --- make check Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks. Thanks, Kapil Arya
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
On July 30, 2015, 9:24 a.m., Adam B wrote: src/slave/slave.cpp, lines 1243-1245 https://reviews.apache.org/r/32587/diff/6/?file=1025122#file1025122line1243 Maybe just warn and leave the CopyFrom there? Next release, we'll remove the field entirely, and consequently this check too. Kapil Arya wrote: With the previous Master (0.23.0), the frameworkInfo.id field is always set, so I am not sure if we need this check at all. Right now, I can't think of a case where we would need this check. Do you have something in mind? Fair enough. I wasn't thinking of a valid use case, just that, generally speaking, a misbehaving sender or invalid message should not kill the slave. Logging an error is probably better here than warning and continuing, if the master is misbehaving or really old. Dropping the issue. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32587/#review93591 --- On July 30, 2015, 9:18 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32587/ --- (Updated July 30, 2015, 9:18 a.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2559 https://issues.apache.org/jira/browse/MESOS-2559 Repository: mesos Description --- FrameworkID can be retrieved from RunTaskMessage.framework. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs - src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510 src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea Diff: https://reviews.apache.org/r/32587/diff/ Testing --- make check Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks. Thanks, Kapil Arya
Re: Review Request 36946: Factored out the pattern for URL generation in a fetcher test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/#review93605 --- src/tests/fetcher_tests.cpp (line 332) https://reviews.apache.org/r/36946/#comment147995 ``` // Tests whether fetcher can process URIs that contain leading whitespace ``` Does this patch broke this? - haosdent huang On July 30, 2015, 4:37 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/ --- (Updated July 30, 2015, 4:37 p.m.) Review request for mesos, Bernd Mathiske and Klaus Ma. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Factored out the pattern for URL generation in (another)fetcher test. Diffs - src/tests/fetcher_tests.cpp 3ded3c037bdfe7095aa15503d81a8d2ee9d420df Diff: https://reviews.apache.org/r/36946/diff/ Testing --- GTEST_FILTER='FetcherTest.OSNetUriSpaceTest' make check Thanks, Artem Harutyunyan
Re: Review Request 36929: Fixed a few issues in test launcher header.
On July 30, 2015, 4:45 p.m., Vinod Kone wrote: src/tests/containerizer/launcher.hpp, lines 19-37 https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19 why did you remove these headers? i think we decided to explicitly include all the headers that are used in a file instead of depending on transitive includes. Is there a discussion somewhere? I think explicitly including all headers make it hard to maintain (you'll need to adjust the header when the dependent header changes). Also, it slows down the compilation. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/#review93599 --- On July 30, 2015, 12:14 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/ --- (Updated July 30, 2015, 12:14 a.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Fixed a few issues in test launcher header. Diffs - src/tests/containerizer/launcher.hpp b80e84196f8b494e6e5a3c96c86f63fe432a7bb0 Diff: https://reviews.apache.org/r/36929/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.
On July 30, 2015, 12:49 a.m., Vinod Kone wrote: Kept the validation error composition per our offline discussion, returning for each case individually led to really verbose code, and we looked at using a lambda to leverage 'return', but this seemed to be the simplest route for now. On July 30, 2015, 12:49 a.m., Vinod Kone wrote: src/master/master.cpp, lines 1574-1576 https://reviews.apache.org/r/36927/diff/1/?file=1024921#file1024921line1574 so this line will be presented twice if principal is not set (which will be for most frameworks) because this function is called twice. that is kinda unfortunate and likely confusing to users. not sure what we could do here yet. Hm.. this is unfortunate, I've pulled it out into the caller code for now to avoid the double logging. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36927/#review93532 --- On July 29, 2015, 11:52 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36927/ --- (Updated July 29, 2015, 11:52 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Framework authorization is now done through `authorizeFramework` which is consistent with `authorizeTask`. Also, `validateFrameworkAuthentication` captures the validation for authentication. I also made registration and re-registration consistent: * Both perform the check for root submission, rather than just registration. * Authentication checks in `_registerFramework` and `_reregisterFramework` are now comprehensive (thanks to `validateFrameworkAuthentication`). Diffs - src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 Diff: https://reviews.apache.org/r/36927/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36927/ --- (Updated July 30, 2015, 10:16 p.m.) Review request for mesos and Vinod Kone. Changes --- Avoid double logging. Repository: mesos Description --- Framework authorization is now done through `authorizeFramework` which is consistent with `authorizeTask`. Also, `validateFrameworkAuthentication` captures the validation for authentication. I also made registration and re-registration consistent: * Both perform the check for root submission, rather than just registration. * Authentication checks in `_registerFramework` and `_reregisterFramework` are now comprehensive (thanks to `validateFrameworkAuthentication`). Diffs (updated) - src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 Diff: https://reviews.apache.org/r/36927/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36625: Windows: Split up platform specific functions into separate headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36625/#review93671 --- Ship it! Ship It! - Benjamin Hindman On July 29, 2015, 11:18 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36625/ --- (Updated July 29, 2015, 11:18 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Alex Clemmer, and Joris Van Remoortere. Bugs: MESOS-3101 https://issues.apache.org/jira/browse/MESOS-3101 Repository: mesos Description --- To support the upcoming Windows Containerizer (MESOS-3094), we're splitting up (refactoring) platform specific functions into separate files. We will avoid having `#ifdef __WINDOWS__` all over the stout/libprcess code by separating Posix/Windows versions. This first patch is to establish a pattern in splitting up the headers. Patterns: * gzip.hpp, thread.hpp - Functions are moved to a Posix folder; copied to a Windows folder and gutted for later implementation. * abort.hpp, exit.hpp, unreachable.hpp - Added macro for `__attribute__((noreturn))`. * duration.hpp - An #ifdef for one of the headers (time.h vs Winsock2.h). No need to split the header. * format.hpp - Missing Windows function (vasprintf) implementation added. * ip.hpp - Added aliases for Windows functions. * net.hpp - Curl functions were moved to Posix/Windows folders. Other: * Instances of #include local file.hpp were changed to #include stout/local file.hpp to match os.hpp. * Some missing #endif comments (i.e. `// __APPLE__`) were added. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 5c19e3ef8ba50ab007eda26b752441f076ca7ed0 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 3aa9487bed2df038ca27a8bb94c24608ca7910a4 3rdparty/libprocess/3rdparty/stout/include/stout/attributes.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp bba8303347aac3f70566a9e69625a928cfb1bd24 3rdparty/libprocess/3rdparty/stout/include/stout/exit.hpp 8c16a224433d7a43bf6bf17e1129e6eb9bbbd573 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 4e8c3bd1e9abf0ff24f78c8385ed9625719dcf8c 3rdparty/libprocess/3rdparty/stout/include/stout/gzip.hpp 0b95819205af6caae05c01cb4d0b25620abe791c 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp a0ea23797376288e8dc96886fd3c0702e5edf846 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a538fb1a343aab039aecabe508b7747e683fd46e 3rdparty/libprocess/3rdparty/stout/include/stout/posix/gzip.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/posix/net.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/posix/thread.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp 552d6e97c882a36d6a889af205c422e51f544b34 3rdparty/libprocess/3rdparty/stout/include/stout/unreachable.hpp fed0a7ba81c98be83a0d66c2317e768877f8e40d 3rdparty/libprocess/3rdparty/stout/include/stout/windows/format.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/windows/gzip.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/windows/net.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/windows/preprocessor.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/windows/thread.hpp PRE-CREATION Diff: https://reviews.apache.org/r/36625/diff/ Testing --- `make` and `make check` (Mac OSX). Build with MSVC Enterprise 2015 [thanks to Alex (hausdorff)]. Thanks, Joseph Wu
Re: Review Request 36929: Fixed a few issues in test launcher header.
On July 30, 2015, 4:45 p.m., Vinod Kone wrote: src/tests/containerizer/launcher.hpp, lines 19-37 https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19 why did you remove these headers? i think we decided to explicitly include all the headers that are used in a file instead of depending on transitive includes. Jie Yu wrote: Is there a discussion somewhere? I think explicitly including all headers make it hard to maintain (you'll need to adjust the header when the dependent header changes). Also, it slows down the compilation. This is the pattern we followed in the code base. I think it improves readability and avoids gotchas. For example, it is not clear at all that the headers you kept provide a string or a vector definition (What if those headers don't #include string at a later time?). This is the relevant blurb from the google style guide ``` You should include all the headers that define the symbols you rely upon (except in cases of forward declaration). If you rely on symbols from bar.h, don't count on the fact that you included foo.h which (currently) includes bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its intent to provide you the symbols of bar.h. However, any includes present in the related header do not need to be included again in the related cc (i.e., foo.cc can rely on foo.h's includes). ``` Some relevant discussion: http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/#review93599 --- On July 30, 2015, 12:14 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/ --- (Updated July 30, 2015, 12:14 a.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Fixed a few issues in test launcher header. Diffs - src/tests/containerizer/launcher.hpp b80e84196f8b494e6e5a3c96c86f63fe432a7bb0 Diff: https://reviews.apache.org/r/36929/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 36929: Fixed a few issues in test launcher header.
On July 30, 2015, 4:45 p.m., Vinod Kone wrote:
src/tests/containerizer/launcher.hpp, lines 19-37
https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19
why did you remove these headers?
i think we decided to explicitly include all the headers that are used
in a file instead of depending on transitive includes.
Jie Yu wrote:
Is there a discussion somewhere? I think explicitly including all headers
make it hard to maintain (you'll need to adjust the header when the dependent
header changes). Also, it slows down the compilation.
Vinod Kone wrote:
This is the pattern we followed in the code base. I think it improves
readability and avoids gotchas. For example, it is not clear at all that the
headers you kept provide a string or a vector definition (What if those
headers don't #include string at a later time?). This is the relevant
blurb from the google style guide
```
You should include all the headers that define the symbols you rely upon
(except in cases of forward declaration). If you rely on symbols from bar.h,
don't count on the fact that you included foo.h which (currently) includes
bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its
intent to provide you the symbols of bar.h. However, any includes present in
the related header do not need to be included again in the related cc (i.e.,
foo.cc can rely on foo.h's includes).
```
Some relevant discussion:
http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
I have to disagree. C++ is not like Java, we don't have an automated tool to
help you include all necessary headers (java does). As a result, that makes
maintaining the includes very tedious. Any time someone is doing a refactor,
he/she has to manually check all the includes to make sure it's up-to-date.
Unless we have an automated tool to help you include all necessary headers, I
think having less includes makes it easy to maintain.
The pattern I suggest here is:
```
base.hpp
#include string
class Base {
virtual void foo(const string str);
};
derived.hpp
#include base.hpp
class Derived : public Base {
virtual void foo(const string str);
};
```
We suggest don't include `string` in derived.hpp. This does not violate
google style as it states: unless foo.h explicitly demonstrates its intent to
provide you the symbols of bar.h. I think here, base.hpp clearly demonstrates
its intent to provide you the symbols for all its public interfaces.
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
---
On July 30, 2015, 12:14 a.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/
---
(Updated July 30, 2015, 12:14 a.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
Fixed a few issues in test launcher header.
Diffs
-
src/tests/containerizer/launcher.hpp
b80e84196f8b494e6e5a3c96c86f63fe432a7bb0
Diff: https://reviews.apache.org/r/36929/diff/
Testing
---
make check
Thanks,
Jie Yu
Re: Review Request 36929: Fixed a few issues in test launcher header.
Well, it does seem easier to maintain includes if we rely on the parent
header demonstrating intent to provide symbols (e.g. adding a vector to an
interface does not require adding includes in all child files).
If it provides significant speedup to build times, it would be very
compelling!
How do tools like include what you use deal with this?
On Thu, Jul 30, 2015 at 10:45 AM, Jie Yu yujie@gmail.com wrote:
On July 30, 2015, 4:45 p.m., Vinod Kone wrote:
src/tests/containerizer/launcher.hpp, lines 19-37
https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19
why did you remove these headers?
i think we decided to explicitly include all the headers that are
used in a file instead of depending on transitive includes.
Jie Yu wrote:
Is there a discussion somewhere? I think explicitly including all
headers make it hard to maintain (you'll need to adjust the header when the
dependent header changes). Also, it slows down the compilation.
Vinod Kone wrote:
This is the pattern we followed in the code base. I think it
improves readability and avoids gotchas. For example, it is not clear at
all that the headers you kept provide a string or a vector definition (What
if those headers don't #include string at a later time?). This is the
relevant blurb from the google style guide
```
You should include all the headers that define the symbols you rely
upon (except in cases of forward declaration). If you rely on symbols from
bar.h, don't count on the fact that you included foo.h which (currently)
includes bar.h: include bar.h yourself, unless foo.h explicitly
demonstrates its intent to provide you the symbols of bar.h. However, any
includes present in the related header do not need to be included again in
the related cc (i.e., foo.cc can rely on foo.h's includes).
```
Some relevant discussion:
http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
I have to disagree. C++ is not like Java, we don't have an automated tool
to help you include all necessary headers (java does). As a result, that
makes maintaining the includes very tedious. Any time someone is doing a
refactor, he/she has to manually check all the includes to make sure it's
up-to-date. Unless we have an automated tool to help you include all
necessary headers, I think having less includes makes it easy to maintain.
The pattern I suggest here is:
```
base.hpp
#include string
class Base {
virtual void foo(const string str);
};
derived.hpp
#include base.hpp
class Derived : public Base {
virtual void foo(const string str);
};
```
We suggest don't include `string` in derived.hpp. This does not violate
google style as it states: unless foo.h explicitly demonstrates its intent
to provide you the symbols of bar.h. I think here, base.hpp clearly
demonstrates its intent to provide you the symbols for all its public
interfaces.
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
---
On July 30, 2015, 12:14 a.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/
---
(Updated July 30, 2015, 12:14 a.m.)
Review request for mesos and Vinod Kone.
Repository: mesos
Description
---
Fixed a few issues in test launcher header.
Diffs
-
src/tests/containerizer/launcher.hpp
b80e84196f8b494e6e5a3c96c86f63fe432a7bb0
Diff: https://reviews.apache.org/r/36929/diff/
Testing
---
make check
Thanks,
Jie Yu
Review Request 36947: Fix new/delete mismatch in stout test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36947/ --- Review request for mesos, Benjamin Hindman, Ben Mahler, Jie Yu, and Vinod Kone. Bugs: MESOS-3175 https://issues.apache.org/jira/browse/MESOS-3175 Repository: mesos Description --- Fix new/delete mismatch in stout test. Diffs - 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36947/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36811: Don't check protobuf jar when --disable-java flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36811/ --- (Updated July 30, 2015, 9:47 a.m.) Review request for mesos, Adam B, Cody Maloney, and Timothy St. Clair. Bugs: MESOS-2480 https://issues.apache.org/jira/browse/MESOS-2480 Repository: mesos Description --- Don't check protobuf jar when --disable-java flag. Diffs (updated) - configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 Diff: https://reviews.apache.org/r/36811/diff/ Testing --- ../configure --with-protobuf=/usr/local --disable-java make -j4 make check ``` [--] Global test environment tear-down [==] 644 tests from 91 test cases ran. (431596 ms total) [ PASSED ] 644 tests. ``` ../configure make -j4 make check ``` [--] Global test environment tear-down [==] 685 tests from 98 test cases ran. (554759 ms total) [ PASSED ] 685 tests. ``` ../configure --disable-java make -j4 make check ``` [--] Global test environment tear-down [==] 644 tests from 91 test cases ran. (427688 ms total) [ PASSED ] 644 tests. ``` ../configure --with-protobuf=/usr/local make -j4 make check ``` [--] Global test environment tear-down [==] 685 tests from 98 test cases ran. (551493 ms total) [ PASSED ] 685 tests. ``` Thanks, haosdent huang
Re: Review Request 36821: Fix disable endpoints rule fails to recognize HTTP path delegates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/#review93567 --- Patch looks great! Reviews applied: [36821] All tests passed. - Mesos ReviewBot On July 30, 2015, 9:29 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/ --- (Updated July 30, 2015, 9:29 a.m.) Review request for mesos, Adam B and Alexander Rojas. Bugs: MESOS-3143 https://issues.apache.org/jira/browse/MESOS-3143 Repository: mesos Description --- Fix disable endpoints rule fails to recognize HTTP path delegates. Diffs - 3rdparty/libprocess/Makefile.am e9c42d7ecd44eebeadcc3d1d5011e01674b3415a 3rdparty/libprocess/include/process/firewall.hpp f92a6c5bbd4af0cab43e487b0c347c30059cf162 3rdparty/libprocess/include/process/process.hpp 8620547148f8a69d5b661eaf08063ca72347b6a4 3rdparty/libprocess/src/firewall.cpp PRE-CREATION 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c Diff: https://reviews.apache.org/r/36821/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 36821: Fix disable endpoints rule fails to recognize HTTP path delegates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/ --- (Updated July 30, 2015, 9:29 a.m.) Review request for mesos, Adam B and Alexander Rojas. Bugs: MESOS-3143 https://issues.apache.org/jira/browse/MESOS-3143 Repository: mesos Description --- Fix disable endpoints rule fails to recognize HTTP path delegates. Diffs (updated) - 3rdparty/libprocess/Makefile.am e9c42d7ecd44eebeadcc3d1d5011e01674b3415a 3rdparty/libprocess/include/process/firewall.hpp f92a6c5bbd4af0cab43e487b0c347c30059cf162 3rdparty/libprocess/include/process/process.hpp 8620547148f8a69d5b661eaf08063ca72347b6a4 3rdparty/libprocess/src/firewall.cpp PRE-CREATION 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c docs/committer-candidate-checklist.md fd55a398c836a30ba80203a64ba10fefe2cb9108 docs/committers.md f37da303552f69aa5a62ecc06fd3077b89360181 src/common/attributes.cpp a8a621e52f0399dbd480437279bdbadf0916f745 src/tests/attributes_tests.cpp ded61204cfe9d086bcafd8a38e99fc5d5ff30390 Diff: https://reviews.apache.org/r/36821/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 36810: Don't check protobuf jar in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36810/#review93565 --- Ship it! LGTM. Any build experts want to take a look? @tstclair, @cmaloney - Adam B On July 28, 2015, 7:43 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36810/ --- (Updated July 28, 2015, 7:43 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2480 https://issues.apache.org/jira/browse/MESOS-2480 Repository: mesos Description --- Don't check protobuf jar in libprocess. Diffs - 3rdparty/libprocess/configure.ac 7d1221bd5ddfc4fa816b0bbea0be5c6b2cbb Diff: https://reviews.apache.org/r/36810/diff/ Testing --- ../configure --with-protobuf=/usr/local --disable-java make -j4 make check -j4 GTEST_FILTER=-* Thanks, haosdent huang
Re: Review Request 36821: Fix disable endpoints rule fails to recognize HTTP path delegates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/#review93561 --- Ship it! Looks great! Unless anybody has any objections, I can remove the `using namespace` and commit this. 3rdparty/libprocess/src/firewall.cpp (line 22) https://reviews.apache.org/r/36821/#comment147941 Shouldn't need to have a `using` for the namespace, since the code is all inside that same namespace. - Adam B On July 29, 2015, 10:40 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/ --- (Updated July 29, 2015, 10:40 a.m.) Review request for mesos, Adam B and Alexander Rojas. Bugs: MESOS-3143 https://issues.apache.org/jira/browse/MESOS-3143 Repository: mesos Description --- Fix disable endpoints rule fails to recognize HTTP path delegates. Diffs - 3rdparty/libprocess/Makefile.am e9c42d7ecd44eebeadcc3d1d5011e01674b3415a 3rdparty/libprocess/include/process/firewall.hpp f92a6c5bbd4af0cab43e487b0c347c30059cf162 3rdparty/libprocess/include/process/process.hpp 8620547148f8a69d5b661eaf08063ca72347b6a4 3rdparty/libprocess/src/firewall.cpp PRE-CREATION 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c Diff: https://reviews.apache.org/r/36821/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 36821: Fix disable endpoints rule fails to recognize HTTP path delegates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36821/ --- (Updated July 30, 2015, 9:29 a.m.) Review request for mesos, Adam B and Alexander Rojas. Bugs: MESOS-3143 https://issues.apache.org/jira/browse/MESOS-3143 Repository: mesos Description --- Fix disable endpoints rule fails to recognize HTTP path delegates. Diffs (updated) - 3rdparty/libprocess/Makefile.am e9c42d7ecd44eebeadcc3d1d5011e01674b3415a 3rdparty/libprocess/include/process/firewall.hpp f92a6c5bbd4af0cab43e487b0c347c30059cf162 3rdparty/libprocess/include/process/process.hpp 8620547148f8a69d5b661eaf08063ca72347b6a4 3rdparty/libprocess/src/firewall.cpp PRE-CREATION 3rdparty/libprocess/src/process.cpp 6d3609d06d017d8e50d7935b335fda7ebecbd04c Diff: https://reviews.apache.org/r/36821/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 36811: Don't check protobuf jar when --disable-java flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36811/#review93570 --- Patch looks great! Reviews applied: [36810, 36811] All tests passed. - Mesos ReviewBot On July 30, 2015, 9:47 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36811/ --- (Updated July 30, 2015, 9:47 a.m.) Review request for mesos, Adam B, Cody Maloney, and Timothy St. Clair. Bugs: MESOS-2480 https://issues.apache.org/jira/browse/MESOS-2480 Repository: mesos Description --- Don't check protobuf jar when --disable-java flag. Diffs - configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 Diff: https://reviews.apache.org/r/36811/diff/ Testing --- ../configure --with-protobuf=/usr/local --disable-java make -j4 make check ``` [--] Global test environment tear-down [==] 644 tests from 91 test cases ran. (431596 ms total) [ PASSED ] 644 tests. ``` ../configure make -j4 make check ``` [--] Global test environment tear-down [==] 685 tests from 98 test cases ran. (554759 ms total) [ PASSED ] 685 tests. ``` ../configure --disable-java make -j4 make check ``` [--] Global test environment tear-down [==] 644 tests from 91 test cases ran. (427688 ms total) [ PASSED ] 644 tests. ``` ../configure --with-protobuf=/usr/local make -j4 make check ``` [--] Global test environment tear-down [==] 685 tests from 98 test cases ran. (551493 ms total) [ PASSED ] 685 tests. ``` Thanks, haosdent huang
