Re: Review Request 35536: Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter.
On June 16, 2015, 10:33 p.m., Jie Yu wrote: src/slave/containerizer/isolators/network/port_mapping.cpp, line 713 https://reviews.apache.org/r/35536/diff/1/?file=985924#file985924line713 I don't think you need to set this. Paul Brett wrote: It is a required field. Without it, the conversion to JSON does not generate a timestamp and hence the conversion back fails. Then, we should use a dummy value here and call clear_timestamp() in the host namespace. Otherwise, it will overwrite the final timestamp set in the containerizer. I'll fix this for you. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/#review88147 --- On June 16, 2015, 11:44 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/ --- (Updated June 16, 2015, 11:44 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2874 https://issues.apache.org/jira/browse/MESOS-2874 Repository: mesos Description --- Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter. Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 Diff: https://reviews.apache.org/r/35536/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 35574: Added missing libraries to mesos-docker-executor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35574/#review88256 --- Ship it! Ship It! - Timothy Chen On June 17, 2015, 6:16 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35574/ --- (Updated June 17, 2015, 6:16 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2881 https://issues.apache.org/jira/browse/MESOS-2881 Repository: mesos Description --- See summary Diffs - src/Makefile.am 884533e4cbb909da81763cf2ae6272177f3d8af3 Diff: https://reviews.apache.org/r/35574/diff/ Testing --- make check (with unbundled dependencies) before and after. Thanks, Niklas Nielsen
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 6:18 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs (updated) - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35229: Report per-container metrics for network bandwidth throttling to the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/ --- (Updated June 17, 2015, 7:25 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2836 https://issues.apache.org/jira/browse/MESOS-2836 Repository: mesos Description --- Report per-container metrics for network bandwidth throttling to the slave. Diffs (updated) - src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df Diff: https://reviews.apache.org/r/35229/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 35229: Report per-container metrics for network bandwidth throttling to the slave.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review88246
---
src/slave/containerizer/isolators/network/port_mapping.cpp (line 683)
https://reviews.apache.org/r/35229/#comment140669
1 extra line please.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 686)
https://reviews.apache.org/r/35229/#comment140676
How about s/CopyNetworkStatsToProtobuf/addTrafficControlStatistics/
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 688 - 689)
https://reviews.apache.org/r/35229/#comment140672
See my comments below.
'statistics' - 'result'
'stats' - 'statistics'
src/slave/containerizer/isolators/network/port_mapping.cpp (line 694)
https://reviews.apache.org/r/35229/#comment140670
Can you include `using namespace routing::queueing::statistics` so that we
don't need to specify the prefix.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 694 - 711)
https://reviews.apache.org/r/35229/#comment140673
```
// TODO: Use protobuf reflection here.
if (statistics.contains(BACKLOG)) {
tc-set_backlog(statistics[BACKLOG]);
}
...
```
src/slave/containerizer/isolators/network/port_mapping.cpp (line 713)
https://reviews.apache.org/r/35229/#comment140668
Extra line please.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 858 - 859)
https://reviews.apache.org/r/35229/#comment140667
Can you pass in eth0_name as we did for PortMappingUpdate? `link::eth0` is
pretty heavy wait (involing reading the routing table, etc.)
src/slave/containerizer/isolators/network/port_mapping.cpp (line 861)
https://reviews.apache.org/r/35229/#comment140671
Hum, we have too many 'stat' here in this file. How about
'statistics' - 'result'
'stats' - 'statistics'
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 863 - 866)
https://reviews.apache.org/r/35229/#comment140674
Do you want to print the error to stderr?
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 869 - 873)
https://reviews.apache.org/r/35229/#comment140675
Ditto on printing the error to stderr.
- Jie Yu
On June 16, 2015, 10 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
---
(Updated June 16, 2015, 10 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2836
https://issues.apache.org/jira/browse/MESOS-2836
Repository: mesos
Description
---
Report per-container metrics for network bandwidth throttling to the slave.
Diffs
-
src/slave/containerizer/isolators/network/port_mapping.hpp
4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp
e55e7b62bc29125458f9c0fb5477057ecc5a90df
Diff: https://reviews.apache.org/r/35229/diff/
Testing
---
sudo make check
Thanks,
Paul Brett
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88266 --- Patch looks great! Reviews applied: [35571] All tests passed. - Mesos ReviewBot On June 17, 2015, 6:18 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 6:18 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35561: Updated process::subprocess to replace environment.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
---
Ship it!
3rdparty/libprocess/src/subprocess.cpp (line 330)
https://reviews.apache.org/r/35561/#comment140700
Aren't we leaking this one in the parent process?
3rdparty/libprocess/src/subprocess.cpp (line 333)
https://reviews.apache.org/r/35561/#comment140693
Should we be using copies here instead?
```
foreachpair (const string key, const string value, environment.get()) {
...
}
```
3rdparty/libprocess/src/tests/subprocess_tests.cpp (line 756)
https://reviews.apache.org/r/35561/#comment140692
I can see that Clock::pause is pretty much everywhere at the top of these
tests. However, I find it not ideal. It should be right before the preparation
of any arguments for, and the invocation of the subprocess.
In this particular test, I would put it after the preparation of the
parent's environment.
- Till Toenshoff
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/
---
(Updated June 17, 2015, 2:28 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/include/process/subprocess.hpp
425b119e5eb122fa21c3e54fe070c7d553958f2c
3rdparty/libprocess/src/subprocess.cpp
f41f5e2a34788e31749eb996c8ab38ea45989068
3rdparty/libprocess/src/tests/subprocess_tests.cpp
b5cfc8d3daff489b332b6bfef2872cef9abecefe
Diff: https://reviews.apache.org/r/35561/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 35229: Report per-container metrics for network bandwidth throttling to the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/ --- (Updated June 17, 2015, 10:58 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2836 https://issues.apache.org/jira/browse/MESOS-2836 Repository: mesos Description --- Report per-container metrics for network bandwidth throttling to the slave. Diffs (updated) - src/slave/containerizer/isolators/network/port_mapping.hpp 2a988b3b3a46aafa6e7f000d8741d01b546bca46 src/slave/containerizer/isolators/network/port_mapping.cpp f97e9608ebe1e72c83c22edb02600fd782db3bca src/tests/port_mapping_tests.cpp 835a0f200dd5452adb3e553367ebde67414276f3 Diff: https://reviews.apache.org/r/35229/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 35257: Decode network statistics from helper
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35257/#review88300 --- Ship it! Ship It! - Jie Yu On June 16, 2015, 10:22 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35257/ --- (Updated June 16, 2015, 10:22 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2837 https://issues.apache.org/jira/browse/MESOS-2837 Repository: mesos Description --- Decode network statistics from helper Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df Diff: https://reviews.apache.org/r/35257/diff/ Testing --- 'sudo make check' for backward compatability Adhoc creation of master/slave with running network task and probing the statistics.json endpoint. Additional tests will be added as part of a later review. Thanks, Paul Brett
Re: Review Request 35571: Adding ability to decode JSON from ZK
On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: Looks great! I inlined a few suggestions below. Will you be following up with a test? I may need some help with how to unit test this - but the short answer is **yes!** :) (I wanted this out ASAP, to avoid wasting too much time if I'd misunderstood people's suggestions) On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: src/master/constants.hpp, line 102 https://reviews.apache.org/r/35571/diff/2/?file=986546#file986546line102 Should we start with 'NOTE:? s/0.24/Mesos 0.24/ Thanks - also below (Mesos 0.23) On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: src/master/detector.cpp, lines 450-473 https://reviews.apache.org/r/35571/diff/2/?file=986548#file986548line450 A bit dense block - can we split up and comment on the flow? Sure - although, the INFO and WARN messages should give a good sense of what's going on? (also, modeled on the rest of the code in this very same method: in fact, the flow is virtually identical, with the addition of the one-line JSON parsing). Added a few blank lines (note: this is violation of Google's preference for vertical space frugality) On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: src/master/detector.cpp, lines 456-457 https://reviews.apache.org/r/35571/diff/2/?file=986548#file986548line456 The wrapping seems off (taken our style guide): How about: ``` promises::fail( promises, Failed to parse data into valid JSON: + jsonMasterInfo.error()); ``` and a few lines above too. On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: src/master/detector.cpp, line 468 https://reviews.apache.org/r/35571/diff/2/?file=986548#file986548line468 Can we inline this in the warning above, so we don't get two log lines? This one is only emitted conditionally, if there is an error. A `Try` (unlike a `Result`), if it's not `some`, can be either `none` or `error`, so we need to cater for both cases. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88276 --- On June 17, 2015, 6:18 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 6:18 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88297 --- Adding a bit to Niq's review... src/master/detector.cpp (lines 465 - 469) https://reviews.apache.org/r/35571/#comment140743 Let's make this a bit more consistent with other error messages. I would suggest this: ``` LOG(WARNING) Could not parse the JSON from ZK into a valid MasterInfo protobuf ' info ' (masterInfo.isError() ? : + masterInfo.error() : ); ``` - Till Toenshoff On June 17, 2015, 10:56 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 10:56 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88307 --- LGTM modulo wrapping at line 450-451. Would love to see a test before we ship this. @vinod - can you take a look at this too? - Niklas Nielsen On June 17, 2015, 4:40 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 4:40 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35353: Smaller fixes on libprocess firewall
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35353/#review88207 --- This looks pretty good - there are just some comment nits I would like to get fixed. 3rdparty/libprocess/include/process/firewall.hpp (line 31) https://reviews.apache.org/r/35353/#comment140602 s/in/is/ ... well actually, lets really use my suggestion unless you got some good reason to not like it :) ``` /** * A 'FirewallRule' describes an interface which provides control * over incoming HTTP requests while also taking the underlying * connection into account. * * Concrete classes based on this interface must implement the * 'apply' method. * * Rules can be installed using the free function * 'process::firewall::install()' defined in 'process.hpp'. */ ``` 3rdparty/libprocess/include/process/firewall.hpp (lines 47 - 58) https://reviews.apache.org/r/35353/#comment140751 Lets boil this one down a little; ``` /** * Verify rule by applying it to an HTTP request and its underlying * socket connection. * * @param socket Socket used to deliver the HTTP request. * @param request HTTP request made by the client to libprocess. * @return If the rule verification fails, i.e. the rule didn't * match, the returned error is set with an explanation for the * failure. Otherwise None is returned. */ ``` 3rdparty/libprocess/include/process/firewall.hpp (line 67) https://reviews.apache.org/r/35353/#comment140752 I think of this class as something that disables any endpoints referenced by paths -- as such, we do not expect *disabled* endpoint paths as parameters. s/disabled// - Till Toenshoff On June 15, 2015, 3:51 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35353/ --- (Updated June 15, 2015, 3:51 p.m.) Review request for mesos, Ben Mahler and Till Toenshoff. Repository: mesos Description --- See summary Diffs - 3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d 3rdparty/libprocess/include/process/process.hpp 6a0b21d67912a40e0ec3220fdb250930be1979b2 3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/35353/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 10:56 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Changes --- nnielsen comments Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs (updated) - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35229: Report per-container metrics for network bandwidth throttling to the slave.
On June 17, 2015, 8:58 p.m., Jie Yu wrote: src/slave/containerizer/isolators/network/port_mapping.cpp, line 703 https://reviews.apache.org/r/35229/diff/5/?file=986593#file986593line703 Please use ``` statistics[BACKLOG]; ``` Since statistics is const, we need to use statistics.at(BACKLOG) etc. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/#review88275 --- On June 17, 2015, 10:58 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/ --- (Updated June 17, 2015, 10:58 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2836 https://issues.apache.org/jira/browse/MESOS-2836 Repository: mesos Description --- Report per-container metrics for network bandwidth throttling to the slave. Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 2a988b3b3a46aafa6e7f000d8741d01b546bca46 src/slave/containerizer/isolators/network/port_mapping.cpp f97e9608ebe1e72c83c22edb02600fd782db3bca src/tests/port_mapping_tests.cpp 835a0f200dd5452adb3e553367ebde67414276f3 Diff: https://reviews.apache.org/r/35229/diff/ Testing --- sudo make check Thanks, Paul Brett
Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 35574: Added missing libraries to mesos-docker-executor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35574/ --- Review request for mesos and Timothy Chen. Bugs: MESOS-2881 https://issues.apache.org/jira/browse/MESOS-2881 Repository: mesos Description --- See summary Diffs - src/Makefile.am 884533e4cbb909da81763cf2ae6272177f3d8af3 Diff: https://reviews.apache.org/r/35574/diff/ Testing --- make check (with unbundled dependencies) before and after. Thanks, Niklas Nielsen
Re: Review Request 35562: Removed unnecessary use of os::ExecEnv.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35562/#review88272
---
Would it make sense to have this RR as being the last in this chain? That way
we could be safely commit earlier RRs without breaking the build.
src/slave/containerizer/mesos/launch.cpp (line 23)
https://reviews.apache.org/r/35562/#comment140702
Seems we can kill this one.
src/slave/containerizer/mesos/launch.cpp (line 36)
https://reviews.apache.org/r/35562/#comment140703
.. and this one :)
src/slave/containerizer/mesos/launch.cpp (line 218)
https://reviews.apache.org/r/35562/#comment140701
Does this TODO refer to a clean environment as being an environment
without anything being set [envp={NULL}]? In any case, does it still make sense
to have this TODO?
- Till Toenshoff
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35562/
---
(Updated June 17, 2015, 2:28 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
src/slave/containerizer/mesos/launch.cpp
2f2d60e2011f60ec711d3b29fd2c157e30c83c34
Diff: https://reviews.apache.org/r/35562/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 30339: Call hookmanager only if some hooks are installed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/ --- (Updated June 17, 2015, 5:06 p.m.) Review request for mesos, Niklas Nielsen and Till Toenshoff. Changes --- Added Jira issue Bugs: MESOS-2883 https://issues.apache.org/jira/browse/MESOS-2883 Repository: mesos Description --- Call hook manager only if hooks were specified on the commandline. Diffs - src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/30339/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88276 --- Looks great! I inlined a few suggestions below. Will you be following up with a test? src/master/constants.hpp (line 102) https://reviews.apache.org/r/35571/#comment140718 Should we start with 'NOTE:? s/0.24/Mesos 0.24/ src/master/detector.cpp (lines 450 - 473) https://reviews.apache.org/r/35571/#comment140720 A bit dense block - can we split up and comment on the flow? src/master/detector.cpp (line 452) https://reviews.apache.org/r/35571/#comment140719 const? src/master/detector.cpp (lines 456 - 457) https://reviews.apache.org/r/35571/#comment140721 The wrapping seems off (taken our style guide): How about: ``` promises::fail( promises, Failed to parse data into valid JSON: + jsonMasterInfo.error()); ``` src/master/detector.cpp (lines 465 - 466) https://reviews.apache.org/r/35571/#comment140722 ``` LOG(WARNING) Could not parse the JSON from ZK into a valid MasterInfo protobuf: info; ``` src/master/detector.cpp (line 468) https://reviews.apache.org/r/35571/#comment140724 Can we inline this in the warning above, so we don't get two log lines? src/master/detector.cpp (lines 471 - 472) https://reviews.apache.org/r/35571/#comment140723 Wrap as above :) - Niklas Nielsen On June 17, 2015, 11:18 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 17, 2015, 11:18 a.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35561: Updated process::subprocess to replace environment.
On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
3rdparty/libprocess/src/subprocess.cpp, line 335
https://reviews.apache.org/r/35561/diff/1/?file=986458#file986458line335
Should we be using copies here instead?
```
foreachpair (const string key, const string value, environment.get()) {
...
}
```
Covered by the styleguide.
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
---
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/
---
(Updated June 17, 2015, 2:28 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/include/process/subprocess.hpp
425b119e5eb122fa21c3e54fe070c7d553958f2c
3rdparty/libprocess/src/subprocess.cpp
f41f5e2a34788e31749eb996c8ab38ea45989068
3rdparty/libprocess/src/tests/subprocess_tests.cpp
b5cfc8d3daff489b332b6bfef2872cef9abecefe
Diff: https://reviews.apache.org/r/35561/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 35563: Removed unused os::ExecEnv.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35563/#review88317 --- Ship it! Ship It! - Till Toenshoff On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35563/ --- (Updated June 17, 2015, 2:28 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 3rdparty/libprocess/3rdparty/stout/include/stout/os/execenv.hpp 1dd6c90f35a5c060d2455fdf399b1fff44ba841b Diff: https://reviews.apache.org/r/35563/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35544: containerizer: statically initialize isolator factories
On June 18, 2015, 3:17 a.m., Michael Park wrote: src/slave/containerizer/mesos/containerizer.cpp, line 135 https://reviews.apache.org/r/35544/diff/1/?file=986152#file986152line135 While your analysis is correct, we need to use `get` here instead since `at` will throw an exception if the element is not found. I see, we're relying on `contains` above to make sure the `at` doesn't throw here... - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35544/#review88329 --- On June 17, 2015, 12:13 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35544/ --- (Updated June 17, 2015, 12:13 a.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- Replaced dynamic hashmap creation with c++11's static initialization. Diffs - src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 Diff: https://reviews.apache.org/r/35544/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 35586: Updated LinuxLauncher to receive list of namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35586/#review88331 --- Patch looks great! Reviews applied: [35585, 35586] All tests passed. - Mesos ReviewBot On June 18, 2015, 1:23 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35586/ --- (Updated June 18, 2015, 1:23 a.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2884 https://issues.apache.org/jira/browse/MESOS-2884 Repository: mesos Description --- MesosContainerizer looks up the list of required namespaces by calling Isolator::namespaces() for all enabled isolators and passes on this value to LinuxLauncher. Diffs - src/slave/containerizer/linux_launcher.hpp ec08e24b9ba525893d218636ebddea480e641bbf src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/tests/isolator_tests.cpp c635a4d5c78d71ca5474993eba57d1f81be9cbf1 Diff: https://reviews.apache.org/r/35586/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 35571: Adding ability to decode JSON from ZK
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35571/#review88315
---
Saw that this got a LGTM, so wanted to help show Nik and Till the kinds of
things to think about and to look out for. :)
src/master/detector.cpp (lines 431 - 436)
https://reviews.apache.org/r/35571/#comment140782
As a follow up for later, is this case still needed?
src/master/detector.cpp (lines 444 - 447)
https://reviews.apache.org/r/35571/#comment140762
Our logging messages do not end with periods, please do a sweep :)
Ditto for aligning on the operator. To be consistent with the rest of
our code base.
Lastly, normally we would put newlines above and below this to make it less
dense for the reader :)
src/master/detector.cpp (lines 450 - 451)
https://reviews.apache.org/r/35571/#comment140763
You don't use on the next line here, but you did above..? Seems
inconsistent, let's use in both cases since that's the general pattern in
the code.
But, let's just remove this comment entirely, it seems unnecessary and it
will show up in 0.23.0 but it says 0.24.0, which may be confusing.
src/master/detector.cpp (line 452)
https://reviews.apache.org/r/35571/#comment140761
Can you just inline it in the parse call? Doesn't think this is buying us
much.
For posterity, we also tend to avoid glueing an assignment right below a
statement without a newline, it tends to read too densely.
Also, we should avoid 'const' proliferation, for example, 'jsonMasterInfo'
and 'masterInfo' can be const here too but that will be pretty dense to read.
src/master/detector.cpp (line 454)
https://reviews.apache.org/r/35571/#comment140765
This makes me wonder, what is the default parser for JSON, do we have
some kind of custom json parser that doesn't follow the RFC..!? :)
To me it's pretty obvious that you're parsing the string as JSON, that's
what the code shows, so I would remove the comment :)
src/master/detector.cpp (line 455)
https://reviews.apache.org/r/35571/#comment140760
(1) s/jsonMasterInfo/object/ or s/jsonMasterInfo/parse/ are common
(2) 'auto' here is not that intuitive and looks inconsistent with the rest
of our code (e.g. you don't use it in protobuf::parse below). In general, we're
trying to use 'auto' where it aids readability, and the type is obvious. Can
you remove it?
src/master/detector.cpp (lines 455 - 456)
https://reviews.apache.org/r/35571/#comment140769
Mind adding a newline after the variable assignment? We've started to do
this in general to improve the readability (less dense as other said).
src/master/detector.cpp (lines 464 - 467)
https://reviews.apache.org/r/35571/#comment140766
Are we going to say this every time we convert from JSON to protobuf? Seems
excessive, let's just remove the comment. :)
src/master/detector.cpp (lines 468 - 470)
https://reviews.apache.org/r/35571/#comment140768
s/masterInfo/info/ to be consistent with the code above :)
Mind adding a newline after the variable assignment? We've started to do
this in general to improve the readability (less dense as other said).
src/master/detector.cpp (lines 470 - 484)
https://reviews.apache.org/r/35571/#comment140776
Any reason not to do the error handling consistently with the json block
above?
```
Trymesos::MasterInfo info = ...;
if (info.isError()) {
...
return;
}
leader = info.get();
```
src/master/detector.cpp (lines 473 - 475)
https://reviews.apache.org/r/35571/#comment140770
You're already returning the failure to the caller, why are you logging it
here as well? The caller with log the failure from the promise.
src/master/detector.cpp (lines 482 - 483)
https://reviews.apache.org/r/35571/#comment140771
This is a strange message, why did you say if any, there is clearly an
error here..! Also, why did you need to have two sentences to say this, when
one sufficed above?
- Ben Mahler
On June 17, 2015, 11:40 p.m., Marco Massenzio wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35571/
---
(Updated June 17, 2015, 11:40 p.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Bugs: MESOS-2340
https://issues.apache.org/jira/browse/MESOS-2340
Repository: mesos
Description
---
Jira: MESOS-2340
The Leader detector will now also try to
read nodes with label master::MASTER_INFO_JSON_LABEL
and parse the contents as JSON, converting the data
to a mesos::MasterInfo protobuf.
The ability to support binary data in PB format is
still supported and the master::Contender will
Re: Review Request 35571: Adding ability to decode JSON from ZK
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88328 --- Patch looks great! Reviews applied: [35571] All tests passed. - Mesos ReviewBot On June 18, 2015, 2:27 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 18, 2015, 2:27 a.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 The Leader detector will now also try to read nodes with label master::MASTER_INFO_JSON_LABEL and parse the contents as JSON, converting the data to a mesos::MasterInfo protobuf. The ability to support binary data in PB format is still supported and the master::Contender will continue to serialize the binary data, so as to be compatible with 0.22 Mesos Masters. Diffs - src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 Diff: https://reviews.apache.org/r/35571/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review88312 --- Ship it! Looks great! I'll get this committed tonight - Adam B On June 15, 2015, 4:23 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 4:23 p.m.) Review request for mesos, Adam B and Cody Maloney. Bugs: MESOS-1733 https://issues.apache.org/jira/browse/MESOS-1733 Repository: mesos Description --- This change takes an un-complicated/naive route ( no trimming of values etc ) at making path::join(...) variadic mainly in order to preserve the earlier over-loaded join functionality. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Review Request 35586: Updated LinuxLauncher to receive list of namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35586/ --- Review request for mesos and Niklas Nielsen. Bugs: MESOS-2884 https://issues.apache.org/jira/browse/MESOS-2884 Repository: mesos Description --- MesosContainerizer looks up the list of required namespaces by calling Isolator::namespaces() for all enabled isolators and passes on this value to LinuxLauncher. Diffs - src/slave/containerizer/linux_launcher.hpp ec08e24b9ba525893d218636ebddea480e641bbf src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/tests/isolator_tests.cpp c635a4d5c78d71ca5474993eba57d1f81be9cbf1 Diff: https://reviews.apache.org/r/35586/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 35571: Adding ability to decode JSON from ZK
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 431-436
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line431
As a follow up for later, is this case still needed?
not sure about this - would you like me to file a Jira or add a TODO?
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 444-447
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line444
Our logging messages do not end with periods, please do a sweep :)
Ditto for aligning on the operator. To be consistent with the rest
of our code base.
Lastly, normally we would put newlines above and below this to make it
less dense for the reader :)
logs are generally meant for parsers and automated tooling (not humans :) -
having multi-line log messages confuses them and is considered to be bad form
in DevOps circles...
aligned indent and removed closing periods.
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 450-451
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line450
You don't use on the next line here, but you did above..? Seems
inconsistent, let's use in both cases since that's the general pattern
in the code.
But, let's just remove this comment entirely, it seems unnecessary and
it will show up in 0.23.0 but it says 0.24.0, which may be confusing.
done
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, line 452
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line452
Can you just inline it in the parse call? Doesn't think this is buying
us much.
For posterity, we also tend to avoid glueing an assignment right below
a statement without a newline, it tends to read too densely.
Also, we should avoid 'const' proliferation, for example,
'jsonMasterInfo' and 'masterInfo' can be const here too but that will be
pretty dense to read.
the `const` was following a comment from @nnielsen
done
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, line 454
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line454
This makes me wonder, what is the default parser for JSON, do we have
some kind of custom json parser that doesn't follow the RFC..!? :)
To me it's pretty obvious that you're parsing the string as JSON,
that's what the code shows, so I would remove the comment :)
yes - here, as below, I've added comments following remarks from @nnielsen that
it was dense for the reader
I may have misunderstood what he meant though.
Will remove, thanks (and below too).
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, line 455
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line455
(1) s/jsonMasterInfo/object/ or s/jsonMasterInfo/parse/ are common
(2) 'auto' here is not that intuitive and looks inconsistent with the
rest of our code (e.g. you don't use it in protobuf::parse below). In
general, we're trying to use 'auto' where it aids readability, and the type
is obvious. Can you remove it?
er... I figured the `JSON::Object` on the other side of the assignment was a
bit of a giveaway :)
done renamed too
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 455-456
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line455
Mind adding a newline after the variable assignment? We've started to
do this in general to improve the readability (less dense as other said).
sure
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 464-467
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line464
Are we going to say this every time we convert from JSON to protobuf?
Seems excessive, let's just remove the comment. :)
I'm with you!
removed
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 470-484
https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line470
Any reason not to do the error handling consistently with the json
block above?
```
Trymesos::MasterInfo info = ...;
if (info.isError()) {
...
return;
}
leader = info.get();
```
yes: this way it groups together both cases where the Try is not Some(): when
it's None() and when there's an Error().
I'm not sure that this will ever happen (in other words, either it succeeds and
it's Some() or it fails with an Error()): but this is one part of the code,
where I'd rather be belt braces than spend hours trying to debug mysterious
bugs...
I think, this is readable enough as is anyway?
On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
src/master/detector.cpp, lines 473-475
Re: Review Request 35565: Refactored os::environment to return a std::map.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35565/#review88319 --- Ship it! Ship It! - Till Toenshoff On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35565/ --- (Updated June 17, 2015, 2:28 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 62987e0df28f28816c59d7cbad89fa2af41ade04 Diff: https://reviews.apache.org/r/35565/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35564: Update process::subprocess callees per new semantics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35564/#review88318 --- Ship it! Ship It! - Till Toenshoff On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35564/ --- (Updated June 17, 2015, 2:28 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/docker/docker.cpp 71383294c6234d08b2156565b170ada329b8e30f src/health-check/main.cpp 3607479848f0e03b0886e3ae84ff92ecb32c33f7 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/containerizer/external_containerizer.cpp 33fc010393e1e664c3b50d849284b14275b29377 src/slave/containerizer/fetcher.cpp f77652b65671c3a63c17960490bbc66d29f5439d src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/tests/slave_tests.cpp 8dae6e0842c2bedddc13d1c036390e85c7a96df7 Diff: https://reviews.apache.org/r/35564/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35566: Refactor executorEnvironment to take slave::Flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35566/#review88320 --- Ship it! src/slave/containerizer/containerizer.hpp (lines 132 - 144) https://reviews.apache.org/r/35566/#comment140781 Nice! src/tests/containerizer.cpp (line 124) https://reviews.apache.org/r/35566/#comment140779 This indention will very soon not comply with our styleguide anymore, no? src/tests/containerizer.cpp (line 136) https://reviews.apache.org/r/35566/#comment140777 See above. - Till Toenshoff On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35566/ --- (Updated June 17, 2015, 2:28 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/containerizer.hpp 0b67d1bdf157b2db927699f44fa8210bacb76701 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/containerizer/docker.hpp 395d5352ad70ed4b7f954d82b1b93e18ca4b74e5 src/slave/containerizer/docker.cpp 00db9811824995fe19a6143aa2bcba3733a93147 src/slave/containerizer/external_containerizer.cpp 33fc010393e1e664c3b50d849284b14275b29377 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/tests/containerizer.cpp 80b910515694c81cf5b1eace1d79a5178cf35426 Diff: https://reviews.apache.org/r/35566/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35229: Report per-container metrics for network bandwidth throttling to the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/#review88275 --- Ship it! Please do a rebase. src/slave/containerizer/isolators/network/port_mapping.cpp (line 696) https://reviews.apache.org/r/35229/#comment140710 Could you please move that to the top of this file? Also, please use fully qualified namespace ``` using namespace routing::queueing::statistics; ``` src/slave/containerizer/isolators/network/port_mapping.cpp (line 703) https://reviews.apache.org/r/35229/#comment140711 Please use ``` statistics[BACKLOG]; ``` src/slave/containerizer/isolators/network/port_mapping.cpp (line 750) https://reviews.apache.org/r/35229/#comment140713 s/results/result/ to be consistent with others. src/slave/containerizer/isolators/network/port_mapping.cpp (line 869) https://reviews.apache.org/r/35229/#comment140712 Can you create a temporary variable since it's used in more than one places: ``` const string eth0 = flags.eth0_name.get(); ``` src/slave/containerizer/isolators/network/port_mapping.cpp (line 879) https://reviews.apache.org/r/35229/#comment140714 enter? - Jie Yu On June 17, 2015, 7:25 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/ --- (Updated June 17, 2015, 7:25 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2836 https://issues.apache.org/jira/browse/MESOS-2836 Repository: mesos Description --- Report per-container metrics for network bandwidth throttling to the slave. Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df Diff: https://reviews.apache.org/r/35229/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 35562: Removed unnecessary use of os::ExecEnv.
On June 17, 2015, 8:32 p.m., Till Toenshoff wrote: Would it make sense to have this RR as being the last in this chain? That way we could be safely commit earlier RRs without breaking the build. Oops, I meant to say the above for the next RR (35563) - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35562/#review88272 --- On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35562/ --- (Updated June 17, 2015, 2:28 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/mesos/launch.cpp 2f2d60e2011f60ec711d3b29fd2c157e30c83c34 Diff: https://reviews.apache.org/r/35562/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/ --- (Updated June 17, 2015, 6:52 a.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2394 https://issues.apache.org/jira/browse/MESOS-2394 Repository: mesos Description --- Introduced General and Markdown Documentation Style Guides. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/README.md 6fc09d6d3cc80c7155a6edc76467c765b160a465 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca docs/mesos-documentation-guide.md PRE-CREATION docs/mesos-markdown-style-guide.md PRE-CREATION Diff: https://reviews.apache.org/r/35510/diff/ Testing --- Rendered Version: Documentation Guide: https://gist.github.com/joerg84/9d1821b745ae8c8f289e Markdown Guide: https://gist.github.com/joerg84/218eb38e066b35a6e25e Thanks, Joerg Schad
Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/#review88200 --- Bad patch! Reviews applied: [35364, 35509, 35510] Failed command: ./support/apply-review.sh -n -r 35510 Error: 2015-06-17 08:00:10 URL:https://reviews.apache.org/r/35510/diff/raw/ [10754/10754] - 35510.patch [1] Successfully applied: Introduced General and Markdown Documentation Style Guides. Introduced General and Markdown Documentation Style Guides. Review: https://reviews.apache.org/r/35510 No files to lint ERROR: Commit spanning multiple projects. Please use separate commits for mesos, libprocess and stout. Paths grouped by project: mesos: docs/home.md docs/mesos-documentation-guide.md docs/mesos-markdown-style-guide.md libprocess: 3rdparty/libprocess/README.md stout: 3rdparty/libprocess/3rdparty/stout/README.md Failed to commit patch - Mesos ReviewBot On June 17, 2015, 6:52 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/ --- (Updated June 17, 2015, 6:52 a.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2394 https://issues.apache.org/jira/browse/MESOS-2394 Repository: mesos Description --- Introduced General and Markdown Documentation Style Guides. Diffs - 3rdparty/libprocess/3rdparty/stout/README.md 6fc09d6d3cc80c7155a6edc76467c765b160a465 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca docs/mesos-documentation-guide.md PRE-CREATION docs/mesos-markdown-style-guide.md PRE-CREATION Diff: https://reviews.apache.org/r/35510/diff/ Testing --- Rendered Version: Documentation Guide: https://gist.github.com/joerg84/9d1821b745ae8c8f289e Markdown Guide: https://gist.github.com/joerg84/218eb38e066b35a6e25e Thanks, Joerg Schad
Review Request 35553: Rename libprocess User Guide to Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35553/ --- Review request for mesos and Bernd Mathiske. Bugs: MESOS-2878 https://issues.apache.org/jira/browse/MESOS-2878 Repository: mesos Description --- Rename libprocess User Guide to Developer Guide. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35553/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 35554: Rename Stout User Guide to Stout Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35554/ --- (Updated June 17, 2015, 9:21 a.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2878 https://issues.apache.org/jira/browse/MESOS-2878 Repository: mesos Description --- Rename Stout User Guide to Stout Developer Guide. Diffs - 3rdparty/libprocess/3rdparty/stout/README.md 6fc09d6d3cc80c7155a6edc76467c765b160a465 Diff: https://reviews.apache.org/r/35554/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/#review88209 --- Patch looks great! Reviews applied: [35364, 35509, 35510] All tests passed. - Mesos ReviewBot On June 17, 2015, 8:22 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/ --- (Updated June 17, 2015, 8:22 a.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2394 https://issues.apache.org/jira/browse/MESOS-2394 Repository: mesos Description --- Introduced General and Markdown Documentation Style Guides. Diffs - docs/home.md ee3d3e9a417d75af776dad3ea9627d3c6d780dca docs/mesos-documentation-guide.md PRE-CREATION docs/mesos-markdown-style-guide.md PRE-CREATION Diff: https://reviews.apache.org/r/35510/diff/ Testing --- Rendered Version: Documentation Guide: https://gist.github.com/joerg84/9d1821b745ae8c8f289e Markdown Guide: https://gist.github.com/joerg84/218eb38e066b35a6e25e Thanks, Joerg Schad
Re: Review Request 34392: Added a method to Path which returns the modification time of the represented path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34392/#review88206 --- 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 35) https://reviews.apache.org/r/34392/#comment140601 Please document what this function does, in particular that it does not follow symlinks and under what conditions it returns an error. - Bernd Mathiske On June 15, 2015, 4:34 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34392/ --- (Updated June 15, 2015, 4:34 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/34392/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35553: Rename libprocess User Guide to Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35553/#review88220 --- Patch looks great! Reviews applied: [35553] All tests passed. - Mesos ReviewBot On June 17, 2015, 9:22 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35553/ --- (Updated June 17, 2015, 9:22 a.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2878 https://issues.apache.org/jira/browse/MESOS-2878 Repository: mesos Description --- Rename libprocess User Guide to Developer Guide. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35553/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 35363: Improvements on libprocess/README.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35363/#review88204 --- 3rdparty/libprocess/README.md (line 19) https://reviews.apache.org/r/35363/#comment140597 I'm not sure of starting a sentence with the prepodisition _to_. What about: … fasion. In order to always be responsive, processes should avoid blocking at all costs. 3rdparty/libprocess/README.md (lines 21 - 23) https://reviews.apache.org/r/35363/#comment140600 Feel free to drop this one, but I was considering to join these two single sentence paragraphs with something like: Each process can be identified symbolically by its [PID](#pid). The ability to locate other processes through their pid allows basic communication between processes with the support of the methods [send](#send), [route](#route) and [install](#install) 3rdparty/libprocess/README.md (line 87) https://reviews.apache.org/r/35363/#comment140612 I think this is missleading or at least confusing. A better description would be: `defer` allows the caller to postpone the decision wether to [dispatch](#dispatch) something by creating a callable object which actually performs the dispatch: I will come with a better example later on. 3rdparty/libprocess/README.md (line 229) https://reviews.apache.org/r/35363/#comment140630 It would be nice to point to some literature about promsises/futures. It would also be nice to describe what happens with broken promises (the promise object is destroyed before setting its value). Boost will throw an exception in this cases. For the latter parts, I think it would be nice to mention a little bit more about the threading model, for example, which thread executes a `then` callback? - Alexander Rojas On June 11, 2015, 10:40 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35363/ --- (Updated June 11, 2015, 10:40 p.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2545 https://issues.apache.org/jira/browse/MESOS-2545 Repository: mesos Description --- Improvements on libprocess/README.md. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35363/diff/ Testing --- Rendered version: https://gist.github.com/joerg84/a6c5fd399bef8cb75b0d Thanks, Joerg Schad
Re: Review Request 35363: Improvements on libprocess/README.md.
On June 17, 2015, 1:58 p.m., Alexander Rojas wrote: Adressed your comments in https://reviews.apache.org/r/35568/. - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35363/#review88204 --- On June 11, 2015, 8:40 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35363/ --- (Updated June 11, 2015, 8:40 p.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2545 https://issues.apache.org/jira/browse/MESOS-2545 Repository: mesos Description --- Improvements on libprocess/README.md. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35363/diff/ Testing --- Rendered version: https://gist.github.com/joerg84/a6c5fd399bef8cb75b0d Thanks, Joerg Schad
Re: Review Request 35363: Improvements on libprocess/README.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35363/#review88226 --- Ship it! Ship It! - Alexander Rojas On June 11, 2015, 10:40 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35363/ --- (Updated June 11, 2015, 10:40 p.m.) Review request for mesos and Bernd Mathiske. Bugs: MESOS-2545 https://issues.apache.org/jira/browse/MESOS-2545 Repository: mesos Description --- Improvements on libprocess/README.md. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35363/diff/ Testing --- Rendered version: https://gist.github.com/joerg84/a6c5fd399bef8cb75b0d Thanks, Joerg Schad
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review88211 --- Well done! An very nice test code. 3rdparty/libprocess/src/process.cpp (line 2815) https://reviews.apache.org/r/30032/#comment140607 Since this review depends on RR 34307 that introduces Path:mtime() you should be using it already! 3rdparty/libprocess/src/process.cpp (line 2817) https://reviews.apache.org/r/30032/#comment140605 That the time could not be parsed is not the only conceivable cause of error. For whatever reason, the asset could be gone, for example. Better to propagate the error from systemMtime.error(). 3rdparty/libprocess/src/process.cpp (line 2818) https://reviews.apache.org/r/30032/#comment140606 If we used a conditional operator (?) expression instead, mtime could be const. 3rdparty/libprocess/src/process.cpp (line 2822) https://reviews.apache.org/r/30032/#comment140611 s/hasn't/has not Or delete the sentence if you handle the next issue as suggested. 3rdparty/libprocess/src/process.cpp (line 2823) https://reviews.apache.org/r/30032/#comment140613 Suggestion to rephrase this: These conditions must hold to establish that the file has not been modified since its most recent access: 1. The If-Modified-Since' header must be present in the request. 2. The local modification time must be determined here. 3. That modification time must differ from the time provided by the above header. An error is logged, but not propagated if any of this fails. Then the request simply proceeds as if the file had been modified. 3rdparty/libprocess/src/process.cpp (line 2826) https://reviews.apache.org/r/30032/#comment140610 I'd move this line up before the comment. 3rdparty/libprocess/src/process.cpp (line 2838) https://reviews.apache.org/r/30032/#comment140614 We could log if client == -1 happens. 3rdparty/libprocess/src/process.cpp (line 2842) https://reviews.apache.org/r/30032/#comment140609 Here we can output mtime.error(). 3rdparty/libprocess/src/process.cpp (line 2846) https://reviews.apache.org/r/30032/#comment140615 Suggestion: Provide the Last-Modified header to the client to facilitate its setting up a cache. 3rdparty/libprocess/src/process.cpp (line 2861) https://reviews.apache.org/r/30032/#comment140624 No need to check mtime.isError(). It is actually a bit confusing all the explanations above. 3rdparty/libprocess/src/process.cpp (line 2862) https://reviews.apache.org/r/30032/#comment140625 Delete this redundant comment. 3rdparty/libprocess/src/process.cpp (line 2862) https://reviews.apache.org/r/30032/#comment140626 Delete this redundant comment. 3rdparty/libprocess/src/tests/process_tests.cpp (line 1734) https://reviews.apache.org/r/30032/#comment140628 seconds 3rdparty/libprocess/src/tests/process_tests.cpp (line 1749) https://reviews.apache.org/r/30032/#comment140631 s/Request/The request's modification s/mismatch/does not match the 3rdparty/libprocess/src/tests/process_tests.cpp (line 1758) https://reviews.apache.org/r/30032/#comment140632 See above. - Bernd Mathiske On June 16, 2015, 7:08 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 16, 2015, 7:08 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417 3rdparty/libprocess/src/process.cpp a67a3afdb30d23eb1b265b04ae662f64e874b6c6 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Review Request 35566: Refactor executorEnvironment to take slave::Flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35566/ --- Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/containerizer.hpp 0b67d1bdf157b2db927699f44fa8210bacb76701 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/containerizer/docker.hpp 395d5352ad70ed4b7f954d82b1b93e18ca4b74e5 src/slave/containerizer/docker.cpp 00db9811824995fe19a6143aa2bcba3733a93147 src/slave/containerizer/external_containerizer.cpp 33fc010393e1e664c3b50d849284b14275b29377 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/tests/containerizer.cpp 80b910515694c81cf5b1eace1d79a5178cf35426 Diff: https://reviews.apache.org/r/35566/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35567: Added 'strip_environment_variables' flag to slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35567/ --- Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/flags.hpp 6c24e56d15881b0e3aeec3d4824842cf57121fc6 src/slave/flags.cpp 93690cfa9dbf2658ce642829299f4adf08bb1062 src/tests/containerizer.cpp 80b910515694c81cf5b1eace1d79a5178cf35426 Diff: https://reviews.apache.org/r/35567/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35565: Refactored os::environment to return a std::map.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35565/ --- Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 62987e0df28f28816c59d7cbad89fa2af41ade04 Diff: https://reviews.apache.org/r/35565/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35568: Remove html from libprocess Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35568/ --- (Updated June 17, 2015, 2:34 p.m.) Review request for mesos and Alexander Rojas. Bugs: MESOS-2545 https://issues.apache.org/jira/browse/MESOS-2545 Repository: mesos Description --- Remove html from libprocess Developer Guide. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35568/diff/ Testing (updated) --- Rendered version: https://gist.github.com/joerg84/9a7df382292dfc672350 Thanks, Joerg Schad
Review Request 35564: Update process::subprocess callees per new semantics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35564/ --- Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/docker/docker.cpp 71383294c6234d08b2156565b170ada329b8e30f src/health-check/main.cpp 3607479848f0e03b0886e3ae84ff92ecb32c33f7 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/containerizer/external_containerizer.cpp 33fc010393e1e664c3b50d849284b14275b29377 src/slave/containerizer/fetcher.cpp f77652b65671c3a63c17960490bbc66d29f5439d src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/tests/slave_tests.cpp 8dae6e0842c2bedddc13d1c036390e85c7a96df7 Diff: https://reviews.apache.org/r/35564/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35563: Removed unused os::ExecEnv.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35563/ --- Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 3rdparty/libprocess/3rdparty/stout/include/stout/os/execenv.hpp 1dd6c90f35a5c060d2455fdf399b1fff44ba841b Diff: https://reviews.apache.org/r/35563/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35562: Removed unnecessary use of os::ExecEnv.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35562/ --- Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/mesos/launch.cpp 2f2d60e2011f60ec711d3b29fd2c157e30c83c34 Diff: https://reviews.apache.org/r/35562/diff/ Testing --- make check Thanks, Benjamin Hindman
Review Request 35568: Remove html from libprocess Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35568/ --- Review request for mesos and Alexander Rojas. Repository: mesos Description --- Remove html from libprocess Developer Guide. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35568/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 17, 2015, 5:42 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Changes --- Addresses Bernd's review Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417 3rdparty/libprocess/src/process.cpp a67a3afdb30d23eb1b265b04ae662f64e874b6c6 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review88240 --- Patch looks great! Reviews applied: [34703, 30032] All tests passed. - Mesos ReviewBot On June 17, 2015, 3:42 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 17, 2015, 3:42 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417 3rdparty/libprocess/src/process.cpp a67a3afdb30d23eb1b265b04ae662f64e874b6c6 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35568: Remove html from libprocess Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35568/ --- (Updated June 17, 2015, 3:24 p.m.) Review request for mesos, Alexander Rojas and Bernd Mathiske. Bugs: MESOS-2545 https://issues.apache.org/jira/browse/MESOS-2545 Repository: mesos Description --- Remove html from libprocess Developer Guide. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35568/diff/ Testing --- Rendered version: https://gist.github.com/joerg84/9a7df382292dfc672350 Thanks, Joerg Schad
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 17, 2015, 5:42 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- Addresses Bernd's comments. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- Adds some manipulator classes which allows formatting Time objects to ostreams. Diffs (updated) - 3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On June 17, 2015, 4 p.m., Bernd Mathiske wrote: 3rdparty/libprocess/src/process.cpp, line 2842 https://reviews.apache.org/r/30032/diff/13/?file=986200#file986200line2842 Here we can output mtime.error(). This one is a bad place since here `strftime` failed, which is unrelated to `mtime`. I did find a better place though. On June 17, 2015, 4 p.m., Bernd Mathiske wrote: 3rdparty/libprocess/src/process.cpp, line 2861 https://reviews.apache.org/r/30032/diff/13/?file=986200#file986200line2861 No need to check mtime.isError(). It is actually a bit confusing all the explanations above. I know the comment above is confusing, however it appears in all the `dispatch(proxy, HttpProxy::enqueue, _, _)` in the file. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review88211 --- On June 17, 2015, 5:42 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 17, 2015, 5:42 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417 3rdparty/libprocess/src/process.cpp a67a3afdb30d23eb1b265b04ae662f64e874b6c6 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35568: Remove html from libprocess Developer Guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35568/#review88234 --- Patch looks great! Reviews applied: [35363, 35568] All tests passed. - Mesos ReviewBot On June 17, 2015, 3:24 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35568/ --- (Updated June 17, 2015, 3:24 p.m.) Review request for mesos, Alexander Rojas and Bernd Mathiske. Bugs: MESOS-2545 https://issues.apache.org/jira/browse/MESOS-2545 Repository: mesos Description --- Remove html from libprocess Developer Guide. Diffs - 3rdparty/libprocess/README.md 4dcd15042dd0b7105c903115cbe8875c3159365e Diff: https://reviews.apache.org/r/35568/diff/ Testing --- Rendered version: https://gist.github.com/joerg84/9a7df382292dfc672350 Thanks, Joerg Schad
