Re: Review Request 36336: Expose major, minor and patch components from stout Version
On July 9, 2015, 2:46 a.m., Kapil Arya wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36 https://reviews.apache.org/r/36336/diff/1/?file=1002978#file1002978line36 Why do we want to rename major/minor/patch to primary/secondary/tertiary? The former is a widely accepted format. g++ defines _GNU_SOURCE (required for libstdc++). This #includes sysmacros.h which #define major and minor as macros for makedev. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91047 --- On July 8, 2015, 10:50 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 8, 2015, 10:50 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line48 It would be good to explain why we are opting this way. How is this useful? OK, maybe nobody knows and we don't want to touch the code? There are various other inexplicable outcomes here. Till Toenshoff wrote: While these may seem inexplicable, they are defined for the standard implementation of this function. I guess what you would like to see is something more along the lines of a `dirname()` and `filename()` to produce results that are more straightforward. I think adding those as an additional implementation is valuable and Cody expressed that as well in the past. Bernd Mathiske wrote: A TODO then? Aye, adding that to Cody's existing TODO. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review90677 --- On June 29, 2015, 11:43 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated June 29, 2015, 11:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp a4afdad0b5f053186ace4d6a37b41cd02e7d415b Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 35687: Added capabilities to state.json
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91113
---
src/master/http.cpp (lines 124 - 125)
https://reviews.apache.org/r/35687/#comment144379
were you not able to use 'foreach' here?
foreach(FrameworkInfo::Capability capability,
framework.info.capabilities()) {
}
src/tests/master_tests.cpp (lines 2994 - 2996)
https://reviews.apache.org/r/35687/#comment144380
pull this below #2998 so that webui_url testing is all in one block.
also, you captured 'Capability' into an JSON::array but doing nothing with
it? did you forget to add an EXPECT_EQ to make sure this value is the same as
the one you added in #2958?
- Vinod Kone
On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/
---
(Updated July 9, 2015, 4:04 p.m.)
Review request for mesos, Marco Massenzio and Vinod Kone.
Bugs: MESOS-2900
https://issues.apache.org/jira/browse/MESOS-2900
Repository: mesos
Description
---
Added capabilities to state.json and test for the same
Diffs
-
src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
Diff: https://reviews.apache.org/r/35687/diff/
Testing
---
make check
Thanks,
Aditi Dixit
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
On July 9, 2015, 5:49 a.m., Joerg Schad wrote: 3rdparty/libprocess/include/process/process.hpp, line 40 https://reviews.apache.org/r/36273/diff/1/?file=1001721#file1001721line40 /s/**Note**:/**NOTE:** The NOTE is already in all caps, as per the Markdown style guide: https://github.com/apache/mesos/blob/master/docs/mesos-markdown-style-guide.md#notesemphasis - Joseph --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/#review91096 --- On July 7, 2015, 5:24 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 7, 2015, 5:24 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35777/ --- (Updated July 9, 2015, 5:07 p.m.) Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till Toenshoff. Changes --- Remove unicode characters copy/pasted from `reviewboardrc` documentation. Repository: mesos Description (updated) --- Handles the case where the ReviewBoard URL is invalid. ``` af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. (29 seconds ago) Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd' ``` Reads `REVIEWBOARD_URL` from `.reviewboardrc` if it's available. We use `imp.load_source` to import the `.reviewboardrc` file. __.reviewboardrc__ The .reviewboardrc file is a generic place for configuring a repository. This must be in a directory in the user's checkout path to work. __It must parse as a valid Python file__, or you'll see an error when using rbt. Diffs - support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 Diff: https://reviews.apache.org/r/35777/diff/ Testing --- Modified the commit message with `git rebase` and ran `./support/post-reviews.py` to observe the above error messages. Used the valid commit message for this patch and created it by running `./support/post-reviews.py`. Thanks, Michael Park
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated July 9, 2015, 4:02 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Changes --- Addressed comments. Repository: mesos Description --- see summary. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 86c12a5 Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91119 --- src/linux/cgroups.hpp (line 445) https://reviews.apache.org/r/36106/#comment144393 1 space after @return and before Some, no? src/linux/cgroups.hpp (lines 473 - 476) https://reviews.apache.org/r/36106/#comment144395 You seem to be using variable space identation for this block unlike the one for the 'cgroup()'. Our styleguide seems to hint to use fixed indentation for these doxygen headers. - Till Toenshoff On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 35687: Added capabilities to state.json
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35687/#review91128 --- Patch looks great! Reviews applied: [35687] All tests passed. - Mesos ReviewBot On July 9, 2015, 4:04 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35687/ --- (Updated July 9, 2015, 4:04 p.m.) Review request for mesos, Marco Massenzio and Vinod Kone. Bugs: MESOS-2900 https://issues.apache.org/jira/browse/MESOS-2900 Repository: mesos Description --- Added capabilities to state.json and test for the same Diffs - src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 Diff: https://reviews.apache.org/r/35687/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review91145 --- src/slave/containerizer/provisioners/appc/hash.hpp (line 40) https://reviews.apache.org/r/34138/#comment144424 A class with only static methods seems weird. Why not put this in a namespace instead? Also, why is this defined here and not in libprocess? src/slave/containerizer/provisioners/appc/hash.hpp (line 61) https://reviews.apache.org/r/34138/#comment144423 This should take a 'Path' type instead of 'string'. src/slave/containerizer/provisioners/appc/hash.hpp (line 69) https://reviews.apache.org/r/34138/#comment144425 looks like 'command()' is only used once here. why split it into a function? - Vinod Kone On July 7, 2015, 7:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 7:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 9, 2015, 6:49 p.m.) Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- Addressed review comments + added a fix to close the pipe upon exit. Would add a test-case for it in a separate review when submitting the re-registration(...) change thereby testing for failover etc. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change lays the ground-work for the master's ability to stream events back to the client. This review turned out a bit too large for my own liking but in a nutshell, it just takes a subscribe request and puts a subscribed event back on the stream. Explanation of changes: - Made a generic FrameworkDriver interface that the master now uses to communicate with the frameworks instead of just invoking send(framework-pid,...) - Framework's can be of 2 types now http, libprocess. - Made a adapter class that takes the messages from master, transforms them to events that the http framework driver can then understand. - This still uses hard-coded http related constants. They can go away when Isabel submits her validation change (36217) - This change prefers use of using trailing under-scores as member variables from the style guide. Diffs (updated) - src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test for subscribe call received a subscribed event back on the stream. Thanks, Anand Mazumdar
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 8, 2015, 6:07 p.m., Isabel Jimenez wrote: src/master/http.cpp, line 316 https://reviews.apache.org/r/36318/diff/1/?file=1002351#file1002351line316 Al the json logic will be handled in the split of 36037 this lines aren't very useful here Left the TODO for now as a place-holder but removed the parsing logic. Not exactly the way you wanted it, but I am expecting this to go away anyways soon. On July 8, 2015, 6:07 p.m., Isabel Jimenez wrote: src/master/master.hpp, line 1584 https://reviews.apache.org/r/36318/diff/1/?file=1002352#file1002352line1584 We should leave this empty line My bad. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review90946 --- On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 9, 2015, 6:49 p.m.) Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change lays the ground-work for the master's ability to stream events back to the client. This review turned out a bit too large for my own liking but in a nutshell, it just takes a subscribe request and puts a subscribed event back on the stream. Explanation of changes: - Made a generic FrameworkDriver interface that the master now uses to communicate with the frameworks instead of just invoking send(framework-pid,...) - Framework's can be of 2 types now http, libprocess. - Made a adapter class that takes the messages from master, transforms them to events that the http framework driver can then understand. - This still uses hard-coded http related constants. They can go away when Isabel submits her validation change (36217) - This change prefers use of using trailing under-scores as member variables from the style guide. Diffs - src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test for subscribe call received a subscribed event back on the stream. Thanks, Anand Mazumdar
Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-3025 https://issues.apache.org/jira/browse/MESOS-3025 Repository: mesos Description --- See MESOS-3025 for details. Diffs - src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 Diff: https://reviews.apache.org/r/36361/diff/ Testing --- make check Existing test coverage encompasses this. Thanks, Ben Mahler
Re: Review Request 34138: AppC hash computation.
On July 9, 2015, 6:47 p.m., Vinod Kone wrote: Also, we need tests!? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review91145 --- On July 7, 2015, 7:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 7:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91129 --- src/tests/cgroups_tests.cpp (line 1191) https://reviews.apache.org/r/36106/#comment144403 Could you please add one or two lines of description for this test as a comment above the test itself? - Till Toenshoff On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/#review91140 --- Patch looks great! Reviews applied: [36273] All tests passed. - Mesos ReviewBot On July 9, 2015, 4:49 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 9, 2015, 4:49 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 34139: AppC image discovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/#review91154 --- tests!? src/slave/containerizer/provisioners/appc/discovery.hpp (line 36) https://reviews.apache.org/r/34139/#comment144435 This class and methods all need commenting! src/slave/containerizer/provisioners/appc/discovery.hpp (line 50) https://reviews.apache.org/r/34139/#comment144437 comments. src/slave/containerizer/provisioners/appc/discovery.hpp (line 68) https://reviews.apache.org/r/34139/#comment144438 comments. src/slave/containerizer/provisioners/appc/discovery.cpp (line 60) https://reviews.apache.org/r/34139/#comment13 +1 please make sure that code in a review only depends on stuff in the current review and its dependent reviews. it's hard to review o.w. - Vinod Kone On July 7, 2015, 7:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/ --- (Updated July 7, 2015, 7:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Local and simple discovery only. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34139/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 35755: Changed fetcher to handle leading whitespace in URLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35755/ --- (Updated July 9, 2015, 10:19 p.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-2862 https://issues.apache.org/jira/browse/MESOS-2862 Repository: mesos Description --- Fixes MESOS-2862 Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 Diff: https://reviews.apache.org/r/35755/diff/ Testing --- - make check - added a test case for fetcher Thanks, Artem Harutyunyan
Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36380/#review91263 --- src/tests/perf_tests.cpp (line 40) https://reviews.apache.org/r/36380/#comment144560 un-used. src/tests/perf_tests.cpp (line 76) https://reviews.apache.org/r/36380/#comment144559 looks like this is 'covered' by the new declartion in the for loop. here and everywhere. - Chi Zhang On July 9, 2015, 11:11 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36380/ --- (Updated July 9, 2015, 11:11 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Test cases for performance monitor support of multiple output versions depending on kernel version. Diffs - src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 Diff: https://reviews.apache.org/r/36380/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36193: Improved Doxygen-Styleguide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36193/#review91086 --- Ship it! Ship It! - Bernd Mathiske On July 6, 2015, 2:01 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36193/ --- (Updated July 6, 2015, 2:01 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Joseph Wu. Repository: mesos Description --- Improved Doxygen-Styleguide for clarifying discussions arising from https://reviews.apache.org/r/36141/. Diffs - docs/mesos-doxygen-style-guide.md 72156c72fc40f72740e57d47d0436c60f6d05bd7 Diff: https://reviews.apache.org/r/36193/diff/ Testing --- Checked rendered markdown. Thanks, Joerg Schad
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
On July 1, 2015, 7:05 a.m., Joerg Schad wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 44 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line44 Did you check how this displays in the rendered version? Good point, looks horrible - using a markdown-styled table fixes it. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review90018 --- On June 29, 2015, 11:43 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated June 29, 2015, 11:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp a4afdad0b5f053186ace4d6a37b41cd02e7d415b Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 36041: The configure phase breaks with the IBM JVM.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36041/#review91071 --- Do we have a JIRA issue that would cover this issue? If so, please add it to this review-request within the Bugs section. If there is no JIRA, please create one that describes the problem briefly. Next question; once the configuration phase is patched like this, do the tests then fully work using the IBM JVM? In other words, right now I don't know if this patch is sufficient to fully support the IBM JVM. - Till Toenshoff On June 30, 2015, 9:09 a.m., Jihun Kang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36041/ --- (Updated June 30, 2015, 9:09 a.m.) Review request for mesos. Repository: mesos Description --- The configure phase breaks with the IBM JVM. Diffs - configure.ac 92909580ea735a1ccffbe03a2eb44e3d07566488 Diff: https://reviews.apache.org/r/36041/diff/ Testing --- Thanks, Jihun Kang
Re: Review Request 23783: Missing Apache headers for libprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23783/#review91073 --- Ship it! Looks great, thanks for solving this tedious task. - Till Toenshoff On July 8, 2015, 7:47 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23783/ --- (Updated July 8, 2015, 7:47 p.m.) Review request for mesos, Benjamin Hindman and Dominic Hamon. Repository: mesos Description --- Adding missing Apache licence headers on .cpp, .hpp and Makefiles Diffs - 3rdparty/libprocess/Makefile.am 358893c 3rdparty/libprocess/configure.ac c197baf 3rdparty/libprocess/examples/example.cpp 3fb4ef5 3rdparty/libprocess/include/Makefile.am d01880c 3rdparty/libprocess/include/process/address.hpp 88946d5 3rdparty/libprocess/include/process/async.hpp 9af3cc0 3rdparty/libprocess/include/process/clock.hpp 8dc7be8 3rdparty/libprocess/include/process/collect.hpp c713c1b 3rdparty/libprocess/include/process/defer.hpp 7c04736 3rdparty/libprocess/include/process/deferred.hpp 3746d69 3rdparty/libprocess/include/process/delay.hpp 29e3532 3rdparty/libprocess/include/process/dispatch.hpp 617fd43 3rdparty/libprocess/include/process/event.hpp ad4a8f4 3rdparty/libprocess/include/process/executor.hpp 157a1d2 3rdparty/libprocess/include/process/filter.hpp aa0c91b 3rdparty/libprocess/include/process/future.hpp a9e765d 3rdparty/libprocess/include/process/gc.hpp e83c636 3rdparty/libprocess/include/process/gmock.hpp 0fd3f8c 3rdparty/libprocess/include/process/gtest.hpp 8518c38 3rdparty/libprocess/include/process/help.hpp 07e99f1 3rdparty/libprocess/include/process/http.hpp 8d9adc5 3rdparty/libprocess/include/process/id.hpp e586937 3rdparty/libprocess/include/process/io.hpp 6388770 3rdparty/libprocess/include/process/latch.hpp 9d010f0 3rdparty/libprocess/include/process/limiter.hpp 444aa1b 3rdparty/libprocess/include/process/logging.hpp 80b1e8f 3rdparty/libprocess/include/process/message.hpp c67c5e1 3rdparty/libprocess/include/process/metrics/counter.hpp f9cab39 3rdparty/libprocess/include/process/metrics/gauge.hpp 7d02cd5 3rdparty/libprocess/include/process/metrics/metric.hpp 616f060 3rdparty/libprocess/include/process/metrics/metrics.hpp aa44992 3rdparty/libprocess/include/process/metrics/timer.hpp 813115a 3rdparty/libprocess/include/process/mime.hpp 0abeac1 3rdparty/libprocess/include/process/mutex.hpp 37ea49a 3rdparty/libprocess/include/process/network.hpp f4192e0 3rdparty/libprocess/include/process/once.hpp 2b81df3 3rdparty/libprocess/include/process/owned.hpp 0541113 3rdparty/libprocess/include/process/pid.hpp e0a9fce 3rdparty/libprocess/include/process/process.hpp 59b50af 3rdparty/libprocess/include/process/profiler.hpp 86050b1 3rdparty/libprocess/include/process/protobuf.hpp 91493de 3rdparty/libprocess/include/process/queue.hpp 5be29db 3rdparty/libprocess/include/process/reap.hpp 5e5051a 3rdparty/libprocess/include/process/run.hpp a0d7286 3rdparty/libprocess/include/process/sequence.hpp d755b34 3rdparty/libprocess/include/process/shared.hpp d80fb7f 3rdparty/libprocess/include/process/socket.hpp 4d95183 3rdparty/libprocess/include/process/statistics.hpp 929fb8d 3rdparty/libprocess/include/process/subprocess.hpp 869e3d9 3rdparty/libprocess/include/process/system.hpp 4fb3c83 3rdparty/libprocess/include/process/time.hpp c5ab2a3 3rdparty/libprocess/include/process/timeout.hpp 5c46d70 3rdparty/libprocess/include/process/timer.hpp e5d71f6 3rdparty/libprocess/include/process/timeseries.hpp ec0ac67 3rdparty/libprocess/src/clock.cpp dd726c1 3rdparty/libprocess/src/config.hpp d5084bf 3rdparty/libprocess/src/decoder.hpp 85ce9e3 3rdparty/libprocess/src/encoder.hpp b898658 3rdparty/libprocess/src/event_loop.hpp af57fe2 3rdparty/libprocess/src/fatal.hpp 34314fd 3rdparty/libprocess/src/fatal.cpp b2934e0 3rdparty/libprocess/src/gate.hpp e5c9379 3rdparty/libprocess/src/http.cpp 0898335 3rdparty/libprocess/src/io.cpp 4944e28 3rdparty/libprocess/src/latch.cpp cba4dcd 3rdparty/libprocess/src/libev.hpp a0a2f49 3rdparty/libprocess/src/libev.cpp 2b8c68d 3rdparty/libprocess/src/libev_poll.cpp 6191be3 3rdparty/libprocess/src/libevent.hpp 47b93f1 3rdparty/libprocess/src/libevent.cpp 67e7501 3rdparty/libprocess/src/libevent_poll.cpp d0b8946 3rdparty/libprocess/src/libevent_ssl_socket.hpp 11c1b70 3rdparty/libprocess/src/libevent_ssl_socket.cpp 9424dd4 3rdparty/libprocess/src/logging.cpp 9134718
Re: Review Request 23784: Missing Apache headers for stout
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23784/#review91074 --- Ship it! 3rdparty/libprocess/3rdparty/stout/configure.ac (lines 13 - 14) https://reviews.apache.org/r/23784/#comment144346 These actually don't add any value, I think. We should get rid of them at some point (not part of this RR!). - Till Toenshoff On July 8, 2015, 8:51 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23784/ --- (Updated July 8, 2015, 8:51 p.m.) Review request for mesos, Benjamin Hindman and Dominic Hamon. Repository: mesos Description --- Adding missing Apache licence headers on .cpp, .hpp and Makefiles Diffs - 3rdparty/libprocess/3rdparty/stout/Makefile.am 9df5de4 3rdparty/libprocess/3rdparty/stout/configure.ac 1988d19 3rdparty/libprocess/3rdparty/stout/include/Makefile.am ff6fb28 3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp c47bd13 3rdparty/libprocess/3rdparty/stout/tests/bytes_tests.cpp 1fe7e08 3rdparty/libprocess/3rdparty/stout/tests/cache_tests.cpp f8d6e42 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 724c5fe 3rdparty/libprocess/3rdparty/stout/tests/error_tests.cpp 35a62b1 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 2a6f67b 3rdparty/libprocess/3rdparty/stout/tests/gzip_tests.cpp 2211f31 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 4a8176b 3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp 97a7167 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp 1fa8fc1 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp c50480b 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 0011f08 3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp 0644d99 3rdparty/libprocess/3rdparty/stout/tests/mac_tests.cpp b80fff9 3rdparty/libprocess/3rdparty/stout/tests/main.cpp f5f20ee 3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 11f3bf7 3rdparty/libprocess/3rdparty/stout/tests/none_tests.cpp 1c1f8be 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp 4a0f60b 3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp fee942c 3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 6d2d3d5 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp d7d4552 3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp dd3700d 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp b3ce131 3rdparty/libprocess/3rdparty/stout/tests/set_tests.cpp b3cd5f8 3rdparty/libprocess/3rdparty/stout/tests/some_tests.cpp ed48279 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 9733b2e 3rdparty/libprocess/3rdparty/stout/tests/thread_tests.cpp 6871e8b 3rdparty/libprocess/3rdparty/stout/tests/uuid_tests.cpp 7ac8fc0 Diff: https://reviews.apache.org/r/23784/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
On July 7, 2015, 2:50 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line48 It would be good to explain why we are opting this way. How is this useful? OK, maybe nobody knows and we don't want to touch the code? There are various other inexplicable outcomes here. Till Toenshoff wrote: While these may seem inexplicable, they are defined for the standard implementation of this function. I guess what you would like to see is something more along the lines of a `dirname()` and `filename()` to produce results that are more straightforward. I think adding those as an additional implementation is valuable and Cody expressed that as well in the past. A TODO then? - Bernd --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review90677 --- On June 29, 2015, 4:43 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated June 29, 2015, 4:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp a4afdad0b5f053186ace4d6a37b41cd02e7d415b Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 39 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line39 Do you mean std::basename()? Nope, the free C function `basename()` is part of the POSIX pattern matching implementations and as such not part of the `std` namespace (not even wrapped). We commonly mark such function using the explicit, global namespace `::` in our C++ codebase. On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 24 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line24 I know that in many other places we have written Basic abstraction, but this really adds no meaning. So let's stop that! And we don't deal with just about any file system here (e.g. FAT32), do we? Suggestion: Represents UNIX file systems paths and offers common path manipulations. Or something like that :-) Yep. Good point, let update that as well. On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line48 It would be good to explain why we are opting this way. How is this useful? OK, maybe nobody knows and we don't want to touch the code? There are various other inexplicable outcomes here. While these may seem inexplicable, they are defined for the standard implementation of this function. I guess what you would like to see is something more along the lines of a `dirname()` and `filename()` to produce results that are more straightforward. I think adding those as an additional implementation is valuable and Cody expressed that as well in the past. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review90677 --- On June 29, 2015, 11:43 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated June 29, 2015, 11:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp a4afdad0b5f053186ace4d6a37b41cd02e7d415b Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 24 https://reviews.apache.org/r/35998/diff/1/?file=994383#file994383line24 I know that in many other places we have written Basic abstraction, but this really adds no meaning. So let's stop that! And we don't deal with just about any file system here (e.g. FAT32), do we? Suggestion: Represents UNIX file systems paths and offers common path manipulations. Or something like that :-) Till Toenshoff wrote: Yep. Good point, let update that as well. s/let/let us/ - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review90677 --- On June 29, 2015, 11:43 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated June 29, 2015, 11:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp a4afdad0b5f053186ace4d6a37b41cd02e7d415b Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 36023: Improved the documentation of Containerizer::launch() to clarify the failure cases.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36023/#review91077 --- Ship it! This is really helpful as it has become a little bit of a confusion milestone for many. - Till Toenshoff On June 29, 2015, 9:29 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36023/ --- (Updated June 29, 2015, 9:29 p.m.) Review request for mesos and Ian Downes. Repository: mesos Description --- Improved the documentation of Containerizer::launch() to clarify the failure cases. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b Diff: https://reviews.apache.org/r/36023/diff/ Testing --- N/A Thanks, Jiang Yan Xu
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review91081 --- src/tests/hierarchical_allocator_tests.cpp (line 785) https://reviews.apache.org/r/35947/#comment144350 Would be great if you could add a brief description as a comment to these tests as done for most others here as well. src/tests/hierarchical_allocator_tests.cpp (line 828) https://reviews.apache.org/r/35947/#comment144351 see above. - Till Toenshoff On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 36216: Only run netcat tests when nc is available.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36216/#review91082 --- Isn't this submitted already? - Joerg Schad On July 6, 2015, 10:58 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36216/ --- (Updated July 6, 2015, 10:58 p.m.) Review request for mesos, Bernd Mathiske, Chi Zhang, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- nc is not available on CentOS 7.1 and all the tests using nc will fail. Added a new NetcatFilter so we detect nc before running these tests. Diffs - src/tests/docker_containerizer_tests.cpp d4ccb0b30fe27980846d913e292d2e18fd3d1055 src/tests/environment.cpp 3726e5d95b9388f0881184e1b4874e1eb2e7f531 src/tests/port_mapping_tests.cpp ac49cdfdcf6baf00ac2907e193c683c8b6c83ffb Diff: https://reviews.apache.org/r/36216/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/#review91096 --- 3rdparty/libprocess/include/process/process.hpp (line 40) https://reviews.apache.org/r/36273/#comment144360 /s/**Note**:/**NOTE:** 3rdparty/libprocess/include/process/process.hpp (line 83) https://reviews.apache.org/r/36273/#comment144361 That seems very long for the brief description... Feel free to drop though... 3rdparty/libprocess/include/process/process.hpp (line 407) https://reviews.apache.org/r/36273/#comment144362 see above 3rdparty/libprocess/include/process/process.hpp (line 423) https://reviews.apache.org/r/36273/#comment144363 the comma seems off here... 3rdparty/libprocess/include/process/process.hpp (line 425) https://reviews.apache.org/r/36273/#comment144364 I know you didn't touch this directly, but as you touched the comment block anyhow could you add a perdiod at the end? - Joerg Schad On July 8, 2015, 12:24 a.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 8, 2015, 12:24 a.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36314: Added test for passing total slave's resources in ResourceUsage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36314/ --- (Updated July 9, 2015, 1:09 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Attaching this request to resolved Mesos issue is not a good idea (: Repository: mesos Description --- Follow up tests for https://reviews.apache.org/r/36204/ Diffs - src/tests/slave_tests.cpp ea0ddf38444915a1cda71cce6a8897803fa49198 Diff: https://reviews.apache.org/r/36314/diff/ Testing --- make check. Thanks, Bartek Plotka
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/#review91100 --- Ship it! Will fix the below issue while comitting. 3rdparty/libprocess/src/time.cpp (line 1) https://reviews.apache.org/r/34703/#comment144366 This misses the “Apache License Version 2.0” header - see our style-guide on File Headers. ``` /** * Licensed under the Apache License, Version 2.0 (the License); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an AS IS BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License */ ``` - Till Toenshoff 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/34703/ --- (Updated June 17, 2015, 3:42 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. 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 - 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review91103 --- Ship it! 3rdparty/libprocess/src/tests/process_tests.cpp (line 1680) https://reviews.apache.org/r/30032/#comment144367 A short description on the target of this test would be terriffic -- will fix this while committing. - Till Toenshoff 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 30032: Added support for cache control in libprocess when dealing with static files.
On July 9, 2015, 7:19 p.m., Ben Mahler wrote: Thanks for looking into this Ben - we will shortly propose a fix for the current test break on Ubuntu and also most of these nits. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review91149 --- 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 36336: Expose major, minor and patch components from stout Version
On July 9, 2015, 2:46 a.m., Kapil Arya wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36 https://reviews.apache.org/r/36336/diff/1/?file=1002978#file1002978line36 Why do we want to rename major/minor/patch to primary/secondary/tertiary? The former is a widely accepted format. Paul Brett wrote: g++ defines _GNU_SOURCE (required for libstdc++). This #includes sysmacros.h which #define major and minor as macros for makedev. Ben Mahler wrote: If you add major(), minor(), patch() as methods instead, does that bypass the macro issue? Paul Brett wrote: No, that doesn't work. At the calling site, x.major() gets expanded to x.gnu_dev_major() by the macro. Kapil Arya wrote: Can we just `#undef` the macros for this file. AFAIK, we aren't using them in the source code. Paul Brett wrote: We could but we would be doing that in a header file which would impact everyone who might ever include stout version directly or indirectly. How about we use getMajor(), getMinor() and getPatch() accessor methods? Per our chat, how about majorVersion(), minorVersion(), patchVersion() with a note for posterity? With getMajor(), seems callers might still be likely to call their variable 'major': `unsigned int major = version.getMajor()` - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91047 --- On July 8, 2015, 10:50 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 8, 2015, 10:50 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91204 --- Patch looks great! Reviews applied: [36360] All tests passed. - Mesos ReviewBot On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36336: Expose major, minor and patch components from stout Version
On July 8, 2015, 10:46 p.m., Kapil Arya wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36 https://reviews.apache.org/r/36336/diff/1/?file=1002978#file1002978line36 Why do we want to rename major/minor/patch to primary/secondary/tertiary? The former is a widely accepted format. Paul Brett wrote: g++ defines _GNU_SOURCE (required for libstdc++). This #includes sysmacros.h which #define major and minor as macros for makedev. Ben Mahler wrote: If you add major(), minor(), patch() as methods instead, does that bypass the macro issue? Paul Brett wrote: No, that doesn't work. At the calling site, x.major() gets expanded to x.gnu_dev_major() by the macro. Can we just `#undef` the macros for this file. AFAIK, we aren't using them in the source code. - Kapil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91047 --- On July 8, 2015, 6:50 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 8, 2015, 6:50 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 438-447 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line438 What is the plan for introducing javadoc comments? cgroups.hpp seems like a reasonable candidate, but we should avoid inconsistent comment formatting within a file :( I'd propose commenting in the existing style, and following up by doing a javadoc formatting sweep across the file, if you're interested. Sound good? I was explicitly asked to do this way for now (see above reviews). I can remove it but will be conflicting with the other reviewer. On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 450-455 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line450 Why are we documenting the cpuacct.stat file format here? The caller should only cares about the user and system times, not the format of the underlying file :) I thought extra information about where that data come from will help. On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 458-466 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line458 Are these kinds of comments providing any value? Just for serving doxygen. Not sure what else I could have added. Sugestions are welcome. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91168 --- On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36360: Adding common constants for HTTP API
On July 9, 2015, 8:23 p.m., Anand Mazumdar wrote: src/common/http_constants.cpp, line 26 https://reviews.apache.org/r/36360/diff/1/?file=1003774#file1003774line26 minor nit-pick , might consider using std::string; before-hand ? :) done - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91178 --- On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs (updated) - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36336: Expose major, minor and patch components from stout Version
On July 9, 2015, 2:46 a.m., Kapil Arya wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36 https://reviews.apache.org/r/36336/diff/1/?file=1002978#file1002978line36 Why do we want to rename major/minor/patch to primary/secondary/tertiary? The former is a widely accepted format. Paul Brett wrote: g++ defines _GNU_SOURCE (required for libstdc++). This #includes sysmacros.h which #define major and minor as macros for makedev. Ben Mahler wrote: If you add major(), minor(), patch() as methods instead, does that bypass the macro issue? Paul Brett wrote: No, that doesn't work. At the calling site, x.major() gets expanded to x.gnu_dev_major() by the macro. Kapil Arya wrote: Can we just `#undef` the macros for this file. AFAIK, we aren't using them in the source code. We could but we would be doing that in a header file which would impact everyone who might ever include stout version directly or indirectly. How about we use getMajor(), getMinor() and getPatch() accessor methods? - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91047 --- On July 8, 2015, 10:50 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 8, 2015, 10:50 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/#review91203 --- Ship it! Can you make sure to test this with a 22.x driver and 23.x master for completeness? - Vinod Kone On July 9, 2015, 6:55 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/ --- (Updated July 9, 2015, 6:55 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-3025 https://issues.apache.org/jira/browse/MESOS-3025 Repository: mesos Description --- See MESOS-3025 for details. Diffs - src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 Diff: https://reviews.apache.org/r/36361/diff/ Testing --- make check Existing test coverage encompasses this. Thanks, Ben Mahler
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91205 --- Can you move this into the existing common/http.hpp, and remove the content type one? For content type, would rather see a typed member on Request/Response than constants here, given the other occurrences: ``` ➜ mesos git:(master) ✗ grep -R Content-Type src | grep -v js | grep -v html src/files/files.cpp: response.headers[Content-Type] = application/octet-stream; src/files/files.cpp: response.headers[Content-Type] = mime::types[extension]; src/tests/fault_tolerance_tests.cpp: Content-Type, src/tests/files_tests.cpp: Content-Type, src/tests/files_tests.cpp: AWAIT_EXPECT_RESPONSE_HEADER_EQ(image/gif, Content-Type, response); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/repair_tests.cpp:Content-Type, \ src/tests/scheduler_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/utils.cpp: response.get().headers.get(Content-Type)); ➜ mesos git:(master) ✗ grep -R Content-Type 3rdparty | grep -v js | grep -v html 3rdparty/libprocess/examples/example.cpp:response.headers[Content-Type] = text/plain; 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/include/process/http.hpp: // specify the 'Content-Type' header, but the 'Content-Length' and 3rdparty/libprocess/include/process/http.hpp: headers[Content-Type] = text/javascript; 3rdparty/libprocess/include/process/process.hpp: // '/path/file'. The 'Content-Type' header of the HTTP response will 3rdparty/libprocess/src/help.cpp:response.headers[Content-Type] = text/x-markdown; 3rdparty/libprocess/src/http.cpp: // Overwrite Content-Type if necessary. 3rdparty/libprocess/src/http.cpp:headers[Content-Type] = contentType.get(); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type from an extension. 3rdparty/libprocess/src/process.cpp:response.headers[Content-Type] = assets[name].types[extension]; 3rdparty/libprocess/src/profiler.cpp: response.headers[Content-Type] = application/octet-stream; 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/http_tests.cpp: headers[Content-Type] = text/plain; 3rdparty/libprocess/src/tests/http_tests.cpp: EXPECT_EQ(text/javascript, response.headers[Content-Type]); 3rdparty/libprocess/src/tests/system_tests.cpp: response.get().headers.get(Content-Type)); ``` - Ben Mahler On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860
Re: Review Request 36336: Expose major, minor and patch components from stout Version
On July 9, 2015, 2:46 a.m., Kapil Arya wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36 https://reviews.apache.org/r/36336/diff/1/?file=1002978#file1002978line36 Why do we want to rename major/minor/patch to primary/secondary/tertiary? The former is a widely accepted format. Paul Brett wrote: g++ defines _GNU_SOURCE (required for libstdc++). This #includes sysmacros.h which #define major and minor as macros for makedev. If you add major(), minor(), patch() as methods instead, does that bypass the macro issue? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91047 --- On July 8, 2015, 10:50 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 8, 2015, 10:50 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91169 --- Patch looks great! Reviews applied: [36318] All tests passed. - Mesos ReviewBot On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 9, 2015, 6:49 p.m.) Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change lays the ground-work for the master's ability to stream events back to the client. This review turned out a bit too large for my own liking but in a nutshell, it just takes a subscribe request and puts a subscribed event back on the stream. Explanation of changes: - Made a generic FrameworkDriver interface that the master now uses to communicate with the frameworks instead of just invoking send(framework-pid,...) - Framework's can be of 2 types now http, libprocess. - Made a adapter class that takes the messages from master, transforms them to events that the http framework driver can then understand. - This still uses hard-coded http related constants. They can go away when Isabel submits her validation change (36217) - This change prefers use of using trailing under-scores as member variables from the style guide. Diffs - src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test for subscribe call received a subscribed event back on the stream. Thanks, Anand Mazumdar
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91168 --- src/linux/cgroups.hpp (lines 438 - 447) https://reviews.apache.org/r/36106/#comment144458 What is the plan for introducing javadoc comments? cgroups.hpp seems like a reasonable candidate, but we should avoid inconsistent comment formatting within a file :( I'd propose commenting in the existing style, and following up by doing a javadoc formatting sweep across the file, if you're interested. Sound good? src/linux/cgroups.hpp (lines 450 - 455) https://reviews.apache.org/r/36106/#comment144457 Why are we documenting the cpuacct.stat file format here? The caller should only cares about the user and system times, not the format of the underlying file :) src/linux/cgroups.hpp (lines 458 - 466) https://reviews.apache.org/r/36106/#comment144456 Are these kinds of comments providing any value? - Ben Mahler On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/#review91190 --- Patch looks great! Reviews applied: [36361] All tests passed. - Mesos ReviewBot On July 9, 2015, 6:55 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/ --- (Updated July 9, 2015, 6:55 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-3025 https://issues.apache.org/jira/browse/MESOS-3025 Repository: mesos Description --- See MESOS-3025 for details. Diffs - src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 Diff: https://reviews.apache.org/r/36361/diff/ Testing --- make check Existing test coverage encompasses this. Thanks, Ben Mahler
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/#review91149
---
3rdparty/libprocess/src/process.cpp (lines 2815 - 2819)
https://reviews.apache.org/r/30032/#comment144432
Any reason we didn't convert os::stat::mtime to return a Time?
The only other user of os::stat::mtime does the same messy conversion:
```
FutureNothing Slave::garbageCollect(const string path)
{
Trylong mtime = os::stat::mtime(path);
if (mtime.isError()) {
LOG(ERROR) Failed to find the mtime of ' path
': mtime.error();
return Failure(mtime.error());
}
// It is unsafe for testing to use unix time directly, we must use
// Time::create to convert into a Time object that reflects the
// possibly advanced state of the libprocess Clock.
TryTime time = Time::create(mtime.get());
CHECK_SOME(time);
```
3rdparty/libprocess/src/process.cpp (line 2827)
https://reviews.apache.org/r/30032/#comment144436
Shouldn't this say if the file hasn't been modified since the requested
time? Note that a comparison becomes possible when dealing with Time rather
than time_t.
3rdparty/libprocess/src/process.cpp (line 2833)
https://reviews.apache.org/r/30032/#comment144431
Why did you need the {} here?
3rdparty/libprocess/src/process.cpp (lines 2839 - 2846)
https://reviews.apache.org/r/30032/#comment144439
Any reason to prefer -1 special cases here to just using TryTime and
handling errors?
3rdparty/libprocess/src/process.cpp (lines 2852 - 2859)
https://reviews.apache.org/r/30032/#comment144429
Why isn't this in the `if (modified)` branch below?
3rdparty/libprocess/src/process.cpp (lines 2857 - 2858)
https://reviews.apache.org/r/30032/#comment144430
Mind lining this up according to the style guide and the code around you?
3rdparty/libprocess/src/tests/process_tests.cpp (line 1680)
https://reviews.apache.org/r/30032/#comment15
How about s/cache/HttpCache/ ?
We should probably start pulling out http server tests.
3rdparty/libprocess/src/tests/process_tests.cpp (line 1711)
https://reviews.apache.org/r/30032/#comment10
std::string here but string above? Mind removing the std:: qualifiers?
3rdparty/libprocess/src/tests/process_tests.cpp (lines 1726 - 1743)
https://reviews.apache.org/r/30032/#comment12
Yikes, could we add library support for parsing into Time to clean this up?
Looks like it could be a lot cleaner.
3rdparty/libprocess/src/tests/process_tests.cpp (lines 1747 - 1751)
https://reviews.apache.org/r/30032/#comment17
Mind just using AWAIT_EXPECT_RESPONSE_STATUS_EQ to clean this up a bit?
Ditto for the other cases here.
- Ben Mahler
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 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36314: Added test for passing total slave's resources in ResourceUsage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36314/#review91162 --- Ship it! LGTM! Some nits. src/tests/slave_tests.cpp (line 2258) https://reviews.apache.org/r/36314/#comment144450 I don't think you need to use the MockExecutor here. src/tests/slave_tests.cpp (line 2262) https://reviews.apache.org/r/36314/#comment144451 No need to use the temp variable. I would suggest the following: ``` slave::Flags flags = CreateSlaveFlage(); flags.resources = cpus:2;mem:1024;disk:1024;ports:[31000-32000]; ... AWAIT_READY(usage); EXPECT_EQ(Resources(usage.get().total()), Resources::parse(flags.resources).get()); ``` src/tests/slave_tests.cpp (line 2280) https://reviews.apache.org/r/36314/#comment144452 This check seems to be redundent. I would suggest killing it. src/tests/slave_tests.cpp (line 2301) https://reviews.apache.org/r/36314/#comment144459 Same. No need for MockExecutor. src/tests/slave_tests.cpp (lines 2305 - 2307) https://reviews.apache.org/r/36314/#comment144460 No need for this temp variable. - Jie Yu On July 9, 2015, 1:09 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36314/ --- (Updated July 9, 2015, 1:09 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Repository: mesos Description --- Follow up tests for https://reviews.apache.org/r/36204/ Diffs - src/tests/slave_tests.cpp ea0ddf38444915a1cda71cce6a8897803fa49198 Diff: https://reviews.apache.org/r/36314/diff/ Testing --- make check. Thanks, Bartek Plotka
Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36336: Expose major, minor and patch components from stout Version
On July 9, 2015, 2:46 a.m., Kapil Arya wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36 https://reviews.apache.org/r/36336/diff/1/?file=1002978#file1002978line36 Why do we want to rename major/minor/patch to primary/secondary/tertiary? The former is a widely accepted format. Paul Brett wrote: g++ defines _GNU_SOURCE (required for libstdc++). This #includes sysmacros.h which #define major and minor as macros for makedev. Ben Mahler wrote: If you add major(), minor(), patch() as methods instead, does that bypass the macro issue? No, that doesn't work. At the calling site, x.major() gets expanded to x.gnu_dev_major() by the macro. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91047 --- On July 8, 2015, 10:50 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 8, 2015, 10:50 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91178 --- Ship it! LGTM, Just some minor comments. src/common/http_constants.hpp (line 18) https://reviews.apache.org/r/36360/#comment144466 This is not a self-contained header file that we encourage everyone to build up, kindly do #include string and remove the adjacent include from the cpp file. src/common/http_constants.hpp (line 26) https://reviews.apache.org/r/36360/#comment144463 s/Supported Content-Tye and Accept headers/Supported media types for Content-Type and Accept headers. src/common/http_constants.cpp (line 26) https://reviews.apache.org/r/36360/#comment144465 minor nit-pick , might consider using std::string; before-hand ? - Anand Mazumdar On July 9, 2015, 7:58 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 7:58 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 8, 2015, 9:57 p.m., Timothy Chen wrote: src/slave/containerizer/docker.hpp, line 230 https://reviews.apache.org/r/36326/diff/3/?file=1002973#file1002973line230 I dont' think we need to expose this. Since the method is a bit long, kept it but replaced other usage methods with lamdas. On July 8, 2015, 9:57 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1239 https://reviews.apache.org/r/36326/diff/3/?file=1002974#file1002974line1239 It's probably not going to be used outside of usage, so I think it's safe to inline this in _usage, and if you do want a seperate method this probably don't need to be exposed too. same comments as above. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91005 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 35687: Added capabilities to state.json
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35687/ --- (Updated July 9, 2015, 4:04 p.m.) Review request for mesos, Marco Massenzio and Vinod Kone. Bugs: MESOS-2900 https://issues.apache.org/jira/browse/MESOS-2900 Repository: mesos Description (updated) --- Added capabilities to state.json and test for the same Diffs (updated) - src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 Diff: https://reviews.apache.org/r/35687/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
On July 8, 2015, 5:59 p.m., Alexander Rukletsov wrote: include/mesos/master/allocator.hpp, lines 133-135 https://reviews.apache.org/r/35947/diff/1/?file=993649#file993649line133 And we introduce a libprocess dependency into `Allocator` interface. I think it's a prominent step, right now an allocator implementation is not even required to be asynchronous. I don't want to say it's a bad change, I want us to think a bit more about the consequences of such step. Maybe it is worth discussing on the dev list. Jie Yu wrote: Most of our module interfaces depends on libprocess (e.g., resource estimator, qos controller, isolator, etc.). Why do you think this is a prominent step? right now an allocator implementation is not even required to be asynchronous I think at the time we introduced modules, we agreed on that internal interfaces are subject to change without needing a formal deprecation cycle. It's up to the module developer to use proper versioning to deal with changes in the interfaces. To clarify, I think it's fine updating an interface, but giving the growing complexity of allocator, introducing a libprocess dependency doesn't make it easier : ). However, if we all share same concerns and are eager to refactor the allocator interface in the near future, I'm fine with the change. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90936 --- On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review91116 --- Patch looks great! Reviews applied: [35998] All tests passed. - Mesos ReviewBot On July 9, 2015, 4:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated July 9, 2015, 4:02 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 86c12a5 Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 9, 2015, 9:49 a.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Changes --- Re-word comment over finalize. Add periods at the end of some @param declarations. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs (updated) - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36360: Adding common constants for HTTP API
On July 9, 2015, 9:32 p.m., Ben Mahler wrote: Can you move this into the existing common/http.hpp, and remove the content type one? For content type, would rather see a typed member on Request/Response than constants here, given the other occurrences: ``` ? mesos git:(master) ? grep -R Content-Type src | grep -v js | grep -v html src/files/files.cpp: response.headers[Content-Type] = application/octet-stream; src/files/files.cpp: response.headers[Content-Type] = mime::types[extension]; src/tests/fault_tolerance_tests.cpp: Content-Type, src/tests/files_tests.cpp: Content-Type, src/tests/files_tests.cpp: AWAIT_EXPECT_RESPONSE_HEADER_EQ(image/gif, Content-Type, response); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/repair_tests.cpp:Content-Type, \ src/tests/scheduler_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/utils.cpp: response.get().headers.get(Content-Type)); ? mesos git:(master) ? grep -R Content-Type 3rdparty | grep -v js | grep -v html 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/include/process/http.hpp: // specify the 'Content-Type' header, but the 'Content-Length' and 3rdparty/libprocess/include/process/http.hpp: headers[Content-Type] = text/javascript; 3rdparty/libprocess/include/process/process.hpp: // '/path/file'. The 'Content-Type' header of the HTTP response will 3rdparty/libprocess/src/help.cpp:response.headers[Content-Type] = text/x-markdown; 3rdparty/libprocess/src/http.cpp: // Overwrite Content-Type if necessary. 3rdparty/libprocess/src/http.cpp:headers[Content-Type] = contentType.get(); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type from an extension. 3rdparty/libprocess/src/process.cpp: response.headers[Content-Type] = assets[name].types[extension]; 3rdparty/libprocess/src/profiler.cpp: response.headers[Content-Type] = application/octet-stream; 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/http_tests.cpp: headers[Content-Type] = text/plain; 3rdparty/libprocess/src/tests/http_tests.cpp: EXPECT_EQ(text/javascript, response.headers[Content-Type]); 3rdparty/libprocess/src/tests/system_tests.cpp: response.get().headers.get(Content-Type)); ``` ok :) - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91205 --- On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/
Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36380/ --- Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Test cases for performance monitor support of multiple output versions depending on kernel version. Diffs - src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 Diff: https://reviews.apache.org/r/36380/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36360: Adding common constants for HTTP API
On July 9, 2015, 9:32 p.m., Ben Mahler wrote: Can you move this into the existing common/http.hpp, and remove the content type one? For content type, would rather see a typed member on Request/Response than constants here, given the other occurrences: ``` ? mesos git:(master) ? grep -R Content-Type src | grep -v js | grep -v html src/files/files.cpp: response.headers[Content-Type] = application/octet-stream; src/files/files.cpp: response.headers[Content-Type] = mime::types[extension]; src/tests/fault_tolerance_tests.cpp: Content-Type, src/tests/files_tests.cpp: Content-Type, src/tests/files_tests.cpp: AWAIT_EXPECT_RESPONSE_HEADER_EQ(image/gif, Content-Type, response); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/repair_tests.cpp:Content-Type, \ src/tests/scheduler_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/utils.cpp: response.get().headers.get(Content-Type)); ? mesos git:(master) ? grep -R Content-Type 3rdparty | grep -v js | grep -v html 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/include/process/http.hpp: // specify the 'Content-Type' header, but the 'Content-Length' and 3rdparty/libprocess/include/process/http.hpp: headers[Content-Type] = text/javascript; 3rdparty/libprocess/include/process/process.hpp: // '/path/file'. The 'Content-Type' header of the HTTP response will 3rdparty/libprocess/src/help.cpp:response.headers[Content-Type] = text/x-markdown; 3rdparty/libprocess/src/http.cpp: // Overwrite Content-Type if necessary. 3rdparty/libprocess/src/http.cpp:headers[Content-Type] = contentType.get(); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type from an extension. 3rdparty/libprocess/src/process.cpp: response.headers[Content-Type] = assets[name].types[extension]; 3rdparty/libprocess/src/profiler.cpp: response.headers[Content-Type] = application/octet-stream; 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/http_tests.cpp: headers[Content-Type] = text/plain; 3rdparty/libprocess/src/tests/http_tests.cpp: EXPECT_EQ(text/javascript, response.headers[Content-Type]); 3rdparty/libprocess/src/tests/system_tests.cpp: response.get().headers.get(Content-Type)); ``` Isabel Jimenez wrote: ok :) LOL - there's something to be said about hard-coded strings :) Thanks, Ben! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91205 --- On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91241 --- Patch looks great! Reviews applied: [36360] All tests passed. - Mesos ReviewBot On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 10:34 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91242 --- Ship it! Ship It! - Marco Massenzio On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 10:34 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 311-316
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line311
unless you know for a fact that none of this will be `None()` you
*must* check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
return BadRequest(Content-Type header MUST be specified);
} else if (contentType.get() == Constants.APPLICATION_JSON) {
return MediaNotSupported(We do not support JSON request/response
content yet);
} else {
...
}
```
Isabel Jimenez wrote:
Hi @marco I commented on previous review that this valdiations will be
handle in the split of the /call patch. That's why it's missing here. It'll
be added with the rebase.
I'm not entirely sure I understand (but I don't really need to): please then
add either a TODO to check the checks (so to speak) or an inline comment
stating the invariant - but, if so, I would like to see a CHECK_SOME() to make
it explicit.
Otherwise, someone else (yourself in 3 months!) may not know/remember about
this and may add redundant checks and/or or remove the others (and leave the
condition unchecked).
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review91213
---
On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/
---
(Updated July 9, 2015, 6:49 p.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and
Vinod Kone.
Bugs: MESOS-2294
https://issues.apache.org/jira/browse/MESOS-2294
Repository: mesos
Description
---
This change lays the ground-work for the master's ability to stream events
back to the client. This review turned out a bit too large for my own liking
but in a nutshell, it just takes a subscribe request and puts a subscribed
event back on the stream.
Explanation of changes:
- Made a generic FrameworkDriver interface that the master now uses to
communicate with the frameworks instead of just invoking
send(framework-pid,...)
- Framework's can be of 2 types now http, libprocess.
- Made a adapter class that takes the messages from master, transforms them
to events that the http framework driver can then understand.
- This still uses hard-coded http related constants. They can go away when
Isabel submits her validation change (36217)
- This change prefers use of using trailing under-scores as member variables
from the style guide.
Diffs
-
src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8
src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1
src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc
src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec
src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294
src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac
Diff: https://reviews.apache.org/r/36318/diff/
Testing
---
make check + a simple test for subscribe call received a subscribed event
back on the stream.
Thanks,
Anand Mazumdar
Re: Review Request 23784: Missing Apache headers for stout
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23784/#review91212 --- Ship it! Ship It! - Benjamin Hindman On July 8, 2015, 8:51 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23784/ --- (Updated July 8, 2015, 8:51 p.m.) Review request for mesos, Benjamin Hindman and Dominic Hamon. Repository: mesos Description --- Adding missing Apache licence headers on .cpp, .hpp and Makefiles Diffs - 3rdparty/libprocess/3rdparty/stout/Makefile.am 9df5de4 3rdparty/libprocess/3rdparty/stout/configure.ac 1988d19 3rdparty/libprocess/3rdparty/stout/include/Makefile.am ff6fb28 3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp c47bd13 3rdparty/libprocess/3rdparty/stout/tests/bytes_tests.cpp 1fe7e08 3rdparty/libprocess/3rdparty/stout/tests/cache_tests.cpp f8d6e42 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 724c5fe 3rdparty/libprocess/3rdparty/stout/tests/error_tests.cpp 35a62b1 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 2a6f67b 3rdparty/libprocess/3rdparty/stout/tests/gzip_tests.cpp 2211f31 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 4a8176b 3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp 97a7167 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp 1fa8fc1 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp c50480b 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 0011f08 3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp 0644d99 3rdparty/libprocess/3rdparty/stout/tests/mac_tests.cpp b80fff9 3rdparty/libprocess/3rdparty/stout/tests/main.cpp f5f20ee 3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 11f3bf7 3rdparty/libprocess/3rdparty/stout/tests/none_tests.cpp 1c1f8be 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp 4a0f60b 3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp fee942c 3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 6d2d3d5 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp d7d4552 3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp dd3700d 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp b3ce131 3rdparty/libprocess/3rdparty/stout/tests/set_tests.cpp b3cd5f8 3rdparty/libprocess/3rdparty/stout/tests/some_tests.cpp ed48279 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 9733b2e 3rdparty/libprocess/3rdparty/stout/tests/thread_tests.cpp 6871e8b 3rdparty/libprocess/3rdparty/stout/tests/uuid_tests.cpp 7ac8fc0 Diff: https://reviews.apache.org/r/23784/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 23783: Missing Apache headers for libprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23783/#review91214 --- Ship it! Ship It! - Benjamin Hindman On July 9, 2015, 9:59 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23783/ --- (Updated July 9, 2015, 9:59 p.m.) Review request for mesos, Benjamin Hindman and Dominic Hamon. Repository: mesos Description --- Adding missing Apache licence headers on .cpp, .hpp and Makefiles Diffs - 3rdparty/libprocess/Makefile.am 71e15f0 3rdparty/libprocess/configure.ac c197baf 3rdparty/libprocess/examples/example.cpp 3fb4ef5 3rdparty/libprocess/include/Makefile.am d01880c 3rdparty/libprocess/include/process/address.hpp 88946d5 3rdparty/libprocess/include/process/async.hpp 9af3cc0 3rdparty/libprocess/include/process/clock.hpp 8dc7be8 3rdparty/libprocess/include/process/collect.hpp c713c1b 3rdparty/libprocess/include/process/defer.hpp 7c04736 3rdparty/libprocess/include/process/deferred.hpp 3746d69 3rdparty/libprocess/include/process/delay.hpp 29e3532 3rdparty/libprocess/include/process/dispatch.hpp 617fd43 3rdparty/libprocess/include/process/event.hpp ad4a8f4 3rdparty/libprocess/include/process/executor.hpp 157a1d2 3rdparty/libprocess/include/process/filter.hpp aa0c91b 3rdparty/libprocess/include/process/future.hpp a9e765d 3rdparty/libprocess/include/process/gc.hpp e83c636 3rdparty/libprocess/include/process/gmock.hpp 0fd3f8c 3rdparty/libprocess/include/process/gtest.hpp afaed51 3rdparty/libprocess/include/process/help.hpp 07e99f1 3rdparty/libprocess/include/process/http.hpp 0db303a 3rdparty/libprocess/include/process/id.hpp e586937 3rdparty/libprocess/include/process/io.hpp 6388770 3rdparty/libprocess/include/process/latch.hpp 9d010f0 3rdparty/libprocess/include/process/limiter.hpp 444aa1b 3rdparty/libprocess/include/process/logging.hpp 80b1e8f 3rdparty/libprocess/include/process/message.hpp c67c5e1 3rdparty/libprocess/include/process/metrics/counter.hpp f9cab39 3rdparty/libprocess/include/process/metrics/gauge.hpp 7d02cd5 3rdparty/libprocess/include/process/metrics/metric.hpp 616f060 3rdparty/libprocess/include/process/metrics/metrics.hpp aa44992 3rdparty/libprocess/include/process/metrics/timer.hpp 813115a 3rdparty/libprocess/include/process/mime.hpp 0abeac1 3rdparty/libprocess/include/process/mutex.hpp 37ea49a 3rdparty/libprocess/include/process/network.hpp f4192e0 3rdparty/libprocess/include/process/once.hpp 2b81df3 3rdparty/libprocess/include/process/owned.hpp 0541113 3rdparty/libprocess/include/process/pid.hpp e0a9fce 3rdparty/libprocess/include/process/process.hpp 59b50af 3rdparty/libprocess/include/process/profiler.hpp 86050b1 3rdparty/libprocess/include/process/protobuf.hpp 91493de 3rdparty/libprocess/include/process/queue.hpp 5be29db 3rdparty/libprocess/include/process/reap.hpp 5e5051a 3rdparty/libprocess/include/process/run.hpp a0d7286 3rdparty/libprocess/include/process/sequence.hpp d755b34 3rdparty/libprocess/include/process/shared.hpp d80fb7f 3rdparty/libprocess/include/process/socket.hpp 4d95183 3rdparty/libprocess/include/process/statistics.hpp 929fb8d 3rdparty/libprocess/include/process/subprocess.hpp 869e3d9 3rdparty/libprocess/include/process/system.hpp 4fb3c83 3rdparty/libprocess/include/process/time.hpp 6a57b7b 3rdparty/libprocess/include/process/timeout.hpp 5c46d70 3rdparty/libprocess/include/process/timer.hpp e5d71f6 3rdparty/libprocess/include/process/timeseries.hpp ec0ac67 3rdparty/libprocess/src/clock.cpp dd726c1 3rdparty/libprocess/src/config.hpp d5084bf 3rdparty/libprocess/src/decoder.hpp 85ce9e3 3rdparty/libprocess/src/encoder.hpp f53fc0d 3rdparty/libprocess/src/event_loop.hpp af57fe2 3rdparty/libprocess/src/fatal.hpp 34314fd 3rdparty/libprocess/src/fatal.cpp b2934e0 3rdparty/libprocess/src/gate.hpp e5c9379 3rdparty/libprocess/src/http.cpp 095572c 3rdparty/libprocess/src/io.cpp 4944e28 3rdparty/libprocess/src/latch.cpp cba4dcd 3rdparty/libprocess/src/libev.hpp a0a2f49 3rdparty/libprocess/src/libev.cpp 2b8c68d 3rdparty/libprocess/src/libev_poll.cpp 6191be3 3rdparty/libprocess/src/libevent.hpp 47b93f1 3rdparty/libprocess/src/libevent.cpp 67e7501 3rdparty/libprocess/src/libevent_poll.cpp d0b8946 3rdparty/libprocess/src/libevent_ssl_socket.hpp 11c1b70 3rdparty/libprocess/src/libevent_ssl_socket.cpp cecd7af 3rdparty/libprocess/src/logging.cpp 9134718 3rdparty/libprocess/src/metrics/metrics.cpp a97b613
Re: Review Request 36336: Expose major, minor and patch components from stout Version
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/#review91218 --- Ship it! Thanks for adding the note Paul! Surprised that these are 'int's rather than 'unsigned int's. - Ben Mahler On July 9, 2015, 10:06 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 9, 2015, 10:06 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/#review91220 --- Ship it! Ship It! - Benjamin Hindman On July 9, 2015, 4:49 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 9, 2015, 4:49 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/#review91223 --- Ship it! Ship It! - Benjamin Hindman On July 8, 2015, 8:23 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/ --- (Updated July 8, 2015, 8:23 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-incubating Description --- Adding missing Apache licence header Diffs - 3rdparty/libprocess/3rdparty/Makefile.am 519e38c Diff: https://reviews.apache.org/r/36226/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review91213
---
Code looks good!
A few general comments (please address them across the entire review - I
stopped making them in every instance):
- no need for leading underscor in argument lists, the private members now have
a trailing one, so no danger of confusion;
- make sure you align the `` in streaming LOGs (same as everywhere in Mesos);
- please make sure that **every** public method has a sufficient Doxygen
javadoc-formatted documentation;
- while we wait for Isabel's http_constants.hpp file to land, please use a
surrogate one, which you can then just replace with an `#include` of the real
one
src/common/protobuf_utils.hpp (lines 78 - 79)
https://reviews.apache.org/r/36318/#comment144492
please use javadoc format
also unnecessary semicolon
s/a/an
(eg an Event)
src/common/protobuf_utils.cpp (line 198)
https://reviews.apache.org/r/36318/#comment144493
an event
also the indent looks off to me?
(either four spaces or line up with quotes - but do you really need to
break the string?)
src/master/http.cpp (line 307)
https://reviews.apache.org/r/36318/#comment144494
while you are at it, do you mind adding javadoc doxy documentation to this
method?
what it does, what the @param's are, what does it return; maybe a link to
the design doc...
as much as you feel like, really: like money and beauty, there's no too
much documentation :)
src/master/http.cpp (lines 311 - 316)
https://reviews.apache.org/r/36318/#comment144497
unless you know for a fact that none of this will be `None()` you *must*
check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
return BadRequest(Content-Type header MUST be specified);
} else if (contentType.get() == Constants.APPLICATION_JSON) {
return MediaNotSupported(We do not support JSON request/response content
yet);
} else {
...
}
```
src/master/http.cpp (line 317)
https://reviews.apache.org/r/36318/#comment144495
the error is actually 415 Media Not Supported (I think - please double
check)
src/master/http.cpp (line 342)
https://reviews.apache.org/r/36318/#comment144498
we should return a `BadRequest` here, shouldn't we? or use UNREACHABLE()
(but that would seem too radical to me: one could crash Mesos with a malformed
request: yay for DOS :)
src/master/http.cpp (lines 1246 - 1249)
https://reviews.apache.org/r/36318/#comment144500
can you please avoid the leading underscore? they seem largely unnecessary
now that we have the trailing ones for the private members
src/master/http.cpp (line 1261)
https://reviews.apache.org/r/36318/#comment144501
no leading underscore
please add doxy for method
src/master/http.cpp (lines 1271 - 1273)
https://reviews.apache.org/r/36318/#comment144502
align (see other code)
src/master/http.cpp (lines 1277 - 1278)
https://reviews.apache.org/r/36318/#comment144503
here too
src/master/master.hpp (line 353)
https://reviews.apache.org/r/36318/#comment144504
use javadoc instead
- Marco Massenzio
On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/
---
(Updated July 9, 2015, 6:49 p.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and
Vinod Kone.
Bugs: MESOS-2294
https://issues.apache.org/jira/browse/MESOS-2294
Repository: mesos
Description
---
This change lays the ground-work for the master's ability to stream events
back to the client. This review turned out a bit too large for my own liking
but in a nutshell, it just takes a subscribe request and puts a subscribed
event back on the stream.
Explanation of changes:
- Made a generic FrameworkDriver interface that the master now uses to
communicate with the frameworks instead of just invoking
send(framework-pid,...)
- Framework's can be of 2 types now http, libprocess.
- Made a adapter class that takes the messages from master, transforms them
to events that the http framework driver can then understand.
- This still uses hard-coded http related constants. They can go away when
Isabel submits her validation change (36217)
- This change prefers use of using trailing under-scores as member variables
from the style guide.
Diffs
-
src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8
src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1
src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc
src/master/master.hpp
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: src/master/http.cpp, line 317 https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line317 the error is actually 415 Media Not Supported (I think - please double check) Same here - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91213 --- On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 9, 2015, 6:49 p.m.) Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change lays the ground-work for the master's ability to stream events back to the client. This review turned out a bit too large for my own liking but in a nutshell, it just takes a subscribe request and puts a subscribed event back on the stream. Explanation of changes: - Made a generic FrameworkDriver interface that the master now uses to communicate with the frameworks instead of just invoking send(framework-pid,...) - Framework's can be of 2 types now http, libprocess. - Made a adapter class that takes the messages from master, transforms them to events that the http framework driver can then understand. - This still uses hard-coded http related constants. They can go away when Isabel submits her validation change (36217) - This change prefers use of using trailing under-scores as member variables from the style guide. Diffs - src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test for subscribe call received a subscribed event back on the stream. Thanks, Anand Mazumdar
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 311-316
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line311
unless you know for a fact that none of this will be `None()` you
*must* check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
return BadRequest(Content-Type header MUST be specified);
} else if (contentType.get() == Constants.APPLICATION_JSON) {
return MediaNotSupported(We do not support JSON request/response
content yet);
} else {
...
}
```
Hi @marco I commented on previous review that this valdiations will be handle
in the split of the /call patch. That's why it's missing here. It'll be added
with the rebase.
- Isabel
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review91213
---
On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/
---
(Updated July 9, 2015, 6:49 p.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and
Vinod Kone.
Bugs: MESOS-2294
https://issues.apache.org/jira/browse/MESOS-2294
Repository: mesos
Description
---
This change lays the ground-work for the master's ability to stream events
back to the client. This review turned out a bit too large for my own liking
but in a nutshell, it just takes a subscribe request and puts a subscribed
event back on the stream.
Explanation of changes:
- Made a generic FrameworkDriver interface that the master now uses to
communicate with the frameworks instead of just invoking
send(framework-pid,...)
- Framework's can be of 2 types now http, libprocess.
- Made a adapter class that takes the messages from master, transforms them
to events that the http framework driver can then understand.
- This still uses hard-coded http related constants. They can go away when
Isabel submits her validation change (36217)
- This change prefers use of using trailing under-scores as member variables
from the style guide.
Diffs
-
src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8
src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1
src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc
src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec
src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294
src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac
Diff: https://reviews.apache.org/r/36318/diff/
Testing
---
make check + a simple test for subscribe call received a subscribed event
back on the stream.
Thanks,
Anand Mazumdar
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/#review91232 --- docs/committers.md (line 11) https://reviews.apache.org/r/36197/#comment144509 No unanimous requirement :) https://community.apache.org/newcommitter.html - Ben Mahler On July 6, 2015, 1:43 p.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/ --- (Updated July 6, 2015, 1:43 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. Bugs: MESOS-1825 https://issues.apache.org/jira/browse/MESOS-1825 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
Re: Review Request 23783: Missing Apache headers for libprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23783/ --- (Updated July 9, 2015, 9:59 p.m.) Review request for mesos, Benjamin Hindman and Dominic Hamon. Repository: mesos Description --- Adding missing Apache licence headers on .cpp, .hpp and Makefiles Diffs (updated) - 3rdparty/libprocess/Makefile.am 71e15f0 3rdparty/libprocess/configure.ac c197baf 3rdparty/libprocess/examples/example.cpp 3fb4ef5 3rdparty/libprocess/include/Makefile.am d01880c 3rdparty/libprocess/include/process/address.hpp 88946d5 3rdparty/libprocess/include/process/async.hpp 9af3cc0 3rdparty/libprocess/include/process/clock.hpp 8dc7be8 3rdparty/libprocess/include/process/collect.hpp c713c1b 3rdparty/libprocess/include/process/defer.hpp 7c04736 3rdparty/libprocess/include/process/deferred.hpp 3746d69 3rdparty/libprocess/include/process/delay.hpp 29e3532 3rdparty/libprocess/include/process/dispatch.hpp 617fd43 3rdparty/libprocess/include/process/event.hpp ad4a8f4 3rdparty/libprocess/include/process/executor.hpp 157a1d2 3rdparty/libprocess/include/process/filter.hpp aa0c91b 3rdparty/libprocess/include/process/future.hpp a9e765d 3rdparty/libprocess/include/process/gc.hpp e83c636 3rdparty/libprocess/include/process/gmock.hpp 0fd3f8c 3rdparty/libprocess/include/process/gtest.hpp afaed51 3rdparty/libprocess/include/process/help.hpp 07e99f1 3rdparty/libprocess/include/process/http.hpp 0db303a 3rdparty/libprocess/include/process/id.hpp e586937 3rdparty/libprocess/include/process/io.hpp 6388770 3rdparty/libprocess/include/process/latch.hpp 9d010f0 3rdparty/libprocess/include/process/limiter.hpp 444aa1b 3rdparty/libprocess/include/process/logging.hpp 80b1e8f 3rdparty/libprocess/include/process/message.hpp c67c5e1 3rdparty/libprocess/include/process/metrics/counter.hpp f9cab39 3rdparty/libprocess/include/process/metrics/gauge.hpp 7d02cd5 3rdparty/libprocess/include/process/metrics/metric.hpp 616f060 3rdparty/libprocess/include/process/metrics/metrics.hpp aa44992 3rdparty/libprocess/include/process/metrics/timer.hpp 813115a 3rdparty/libprocess/include/process/mime.hpp 0abeac1 3rdparty/libprocess/include/process/mutex.hpp 37ea49a 3rdparty/libprocess/include/process/network.hpp f4192e0 3rdparty/libprocess/include/process/once.hpp 2b81df3 3rdparty/libprocess/include/process/owned.hpp 0541113 3rdparty/libprocess/include/process/pid.hpp e0a9fce 3rdparty/libprocess/include/process/process.hpp 59b50af 3rdparty/libprocess/include/process/profiler.hpp 86050b1 3rdparty/libprocess/include/process/protobuf.hpp 91493de 3rdparty/libprocess/include/process/queue.hpp 5be29db 3rdparty/libprocess/include/process/reap.hpp 5e5051a 3rdparty/libprocess/include/process/run.hpp a0d7286 3rdparty/libprocess/include/process/sequence.hpp d755b34 3rdparty/libprocess/include/process/shared.hpp d80fb7f 3rdparty/libprocess/include/process/socket.hpp 4d95183 3rdparty/libprocess/include/process/statistics.hpp 929fb8d 3rdparty/libprocess/include/process/subprocess.hpp 869e3d9 3rdparty/libprocess/include/process/system.hpp 4fb3c83 3rdparty/libprocess/include/process/time.hpp 6a57b7b 3rdparty/libprocess/include/process/timeout.hpp 5c46d70 3rdparty/libprocess/include/process/timer.hpp e5d71f6 3rdparty/libprocess/include/process/timeseries.hpp ec0ac67 3rdparty/libprocess/src/clock.cpp dd726c1 3rdparty/libprocess/src/config.hpp d5084bf 3rdparty/libprocess/src/decoder.hpp 85ce9e3 3rdparty/libprocess/src/encoder.hpp f53fc0d 3rdparty/libprocess/src/event_loop.hpp af57fe2 3rdparty/libprocess/src/fatal.hpp 34314fd 3rdparty/libprocess/src/fatal.cpp b2934e0 3rdparty/libprocess/src/gate.hpp e5c9379 3rdparty/libprocess/src/http.cpp 095572c 3rdparty/libprocess/src/io.cpp 4944e28 3rdparty/libprocess/src/latch.cpp cba4dcd 3rdparty/libprocess/src/libev.hpp a0a2f49 3rdparty/libprocess/src/libev.cpp 2b8c68d 3rdparty/libprocess/src/libev_poll.cpp 6191be3 3rdparty/libprocess/src/libevent.hpp 47b93f1 3rdparty/libprocess/src/libevent.cpp 67e7501 3rdparty/libprocess/src/libevent_poll.cpp d0b8946 3rdparty/libprocess/src/libevent_ssl_socket.hpp 11c1b70 3rdparty/libprocess/src/libevent_ssl_socket.cpp cecd7af 3rdparty/libprocess/src/logging.cpp 9134718 3rdparty/libprocess/src/metrics/metrics.cpp a97b613 3rdparty/libprocess/src/openssl.hpp 16f3d56 3rdparty/libprocess/src/openssl.cpp 118ce55 3rdparty/libprocess/src/openssl_util.hpp f855e27 3rdparty/libprocess/src/openssl_util.cpp cd38f17 3rdparty/libprocess/src/pid.cpp 979c370 3rdparty/libprocess/src/poll_socket.hpp a14d63f 3rdparty/libprocess/src/poll_socket.cpp 9cb4ef9 3rdparty/libprocess/src/process.cpp 9037b98
Re: Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.
On July 9, 2015, 9:26 p.m., Vinod Kone wrote: Can you make sure to test this with a 22.x driver and 23.x master for completeness? Good call, did a manual test by modifying the test framework in 0.22.0 to do reconciliation against a master off head with this fix, parses the message correctly: ``` $ ./src/test-framework --master=10.35.255.108:5050 I0709 22:00:26.220700 14160 sched.cpp:157] Version: 0.22.0 I0709 22:00:26.228389 14202 sched.cpp:254] New master detected at master@10.35.255.108:5050 I0709 22:00:26.229192 14202 sched.cpp:264] No credentials provided. Attempting to register without authentication I0709 22:00:26.233379 14210 sched.cpp:448] Framework registered with 20150709-214514-1828659978-5050-3855-0001 Registered! Task 1 is in state TASK_LOST Aborting because task 1 is in unexpected state TASK_LOST with reason 9 from source 0 with message 'Reconciliation: Task is unknown' I0709 22:00:26.235369 14212 sched.cpp:1623] Asked to abort the driver I0709 22:00:26.235436 14212 sched.cpp:856] Aborting framework '20150709-214514-1828659978-5050-3855-0001' I0709 22:00:26.235656 14160 sched.cpp:1589] Asked to stop the driver ``` - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/#review91203 --- On July 9, 2015, 6:55 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36361/ --- (Updated July 9, 2015, 6:55 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-3025 https://issues.apache.org/jira/browse/MESOS-3025 Repository: mesos Description --- See MESOS-3025 for details. Diffs - src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 Diff: https://reviews.apache.org/r/36361/diff/ Testing --- make check Existing test coverage encompasses this. Thanks, Ben Mahler
Re: Review Request 36336: Expose major, minor and patch components from stout Version
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36336/ --- (Updated July 9, 2015, 10:06 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, Kapil Arya, and Vinod Kone. Changes --- Incorporate review comments. Bugs: MESOS-3020 https://issues.apache.org/jira/browse/MESOS-3020 Repository: mesos Description --- Expose major, minor and patch components from stout Version Note: The names major and minor are not available when compiling with GCC because it pulls in the sysmacros.h header implicitly which define these for makedev. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 8692323d28131cd5706dde0503d49f8f0b0a1aeb Diff: https://reviews.apache.org/r/36336/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36218: Doxygen styleguide revisions based on conversation from https://reviews.apache.org/r/36193/.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36218/#review91219 --- Ship it! Ship It! - Benjamin Hindman On July 8, 2015, 8:45 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36218/ --- (Updated July 8, 2015, 8:45 p.m.) Review request for mesos, Benjamin Hindman and Joerg Schad. Repository: mesos Description --- Doxygen styleguide revisions based on conversation from https://reviews.apache.org/r/36193/. Diffs - docs/mesos-doxygen-style-guide.md 72156c72fc40f72740e57d47d0436c60f6d05bd7 Diff: https://reviews.apache.org/r/36218/diff/ Testing --- Checked rendered markdown. Thanks, Joseph Wu
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91224 --- Patch looks great! Reviews applied: [36106, 36326] All tests passed. - Mesos ReviewBot On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
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/#review91225 --- Ship it! LGTM Bernd, let's commit this and ammend as necessary in the future. Thanks! - Benjamin Hindman On July 6, 2015, 1:43 p.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/ --- (Updated July 6, 2015, 1:43 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. Bugs: MESOS-1825 https://issues.apache.org/jira/browse/MESOS-1825 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91230 --- src/master/http.cpp (line 342) https://reviews.apache.org/r/36318/#comment144508 Same here. - Isabel Jimenez On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 9, 2015, 6:49 p.m.) Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change lays the ground-work for the master's ability to stream events back to the client. This review turned out a bit too large for my own liking but in a nutshell, it just takes a subscribe request and puts a subscribed event back on the stream. Explanation of changes: - Made a generic FrameworkDriver interface that the master now uses to communicate with the frameworks instead of just invoking send(framework-pid,...) - Framework's can be of 2 types now http, libprocess. - Made a adapter class that takes the messages from master, transforms them to events that the http framework driver can then understand. - This still uses hard-coded http related constants. They can go away when Isabel submits her validation change (36217) - This change prefers use of using trailing under-scores as member variables from the style guide. Diffs - src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test for subscribe call received a subscribed event back on the stream. Thanks, Anand Mazumdar
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
Code looks good!
A few general comments (please address them across the entire review - I
stopped making them in every instance):
- no need for leading underscor in argument lists, the private members now
have a trailing one, so no danger of confusion;
- make sure you align the `` in streaming LOGs (same as everywhere in
Mesos);
- please make sure that **every** public method has a sufficient Doxygen
javadoc-formatted documentation;
- while we wait for Isabel's http_constants.hpp file to land, please use a
surrogate one, which you can then just replace with an `#include` of the
real one
Ben Mahler wrote:
I'll shepherd this for Anand, IMO this change needs to go through a
higher level review first, otherwise we're likely to go through a ton of
iterations (see part (1) of 'Reviewing' here):
http://mesos.apache.org/documentation/latest/effective-code-reviewing/
Thanks Ben for agreeing to shepherd this. Looking forward to your inputs and
discussing the change in greater detail.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 307
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line307
while you are at it, do you mind adding javadoc doxy documentation to
this method?
what it does, what the @param's are, what does it return; maybe a link
to the design doc...
as much as you feel like, really: like money and beauty, there's no too
much documentation :)
There is already a TODO on the CALL_HELP variable for documenting this better.
This can easily be pursued as part of that. Do you still want me to pursue this
as part of this review ?
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 311-316
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line311
unless you know for a fact that none of this will be `None()` you
*must* check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
return BadRequest(Content-Type header MUST be specified);
} else if (contentType.get() == Constants.APPLICATION_JSON) {
return MediaNotSupported(We do not support JSON request/response
content yet);
} else {
...
}
```
Isabel Jimenez wrote:
Hi @marco I commented on previous review that this valdiations will be
handle in the split of the /call patch. That's why it's missing here. It'll
be added with the rebase.
Marco Massenzio wrote:
I'm not entirely sure I understand (but I don't really need to): please
then add either a TODO to check the checks (so to speak) or an inline comment
stating the invariant - but, if so, I would like to see a CHECK_SOME() to
make it explicit.
Otherwise, someone else (yourself in 3 months!) may not know/remember
about this and may add redundant checks and/or or remove the others (and
leave the condition unchecked).
My bad, I should have added a better TODO instead of the vague one that says
Fix logic when we start supporting application/json. I will add a better
TODO. I was under the impression that all this would be taken care as part of
the validations patch.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 1246-1249
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1246
can you please avoid the leading underscore? they seem largely
unnecessary now that we have the trailing ones for the private members
From the style guide :
- We prepend constructor and function arguments with a leading underscore to
avoid ambiguity and / or shadowing:
- Prefer trailing underscores for use as member fields (but not required).
Some trailing underscores are used to distinguish between similar variables in
the same scope (think prime symbols), but this should be avoided as much as
possible, including removing existing instances in the code base.
Seems like I am missing something here, I don't find any reason to follow one
and discard the other i.e. not use both at once. If not, these need to be
better explained in the style guide. What do you think ?
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 1261
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1261
no leading underscore
please add doxy for method
The _ was added as it was needed for resolving ambiguity with the already
declared event variable used in the function.
Also it already has a explanation of what the method does in the .hpp file
base-class. What would we gain by adding documentation again here ?
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 1271-1273
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1271
align (see other code)
My bad, let me fix all of these.
On July 9,
