Re: Review Request 34881: let libprocess to compile on arm64 servers
On 六月 22, 2015, 3:48 a.m., Cody Maloney wrote: 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch, line 18 https://reviews.apache.org/r/34881/diff/1/?file=975553#file975553line18 Where did this header come from? If we were to just update protobuf to a newer version would it just compile? This header comes from https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/atomicops_internals_generic_gcc.h, the latest protobuf code. If we update the protobuf to the newer version, it would just compile. And, if we update protobuf to the latest version (which already supported arm64 arch), we don't need this patch anymore. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34881/#review88722 --- On 六月 1, 2015, 7:10 a.m., Dong Robin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34881/ --- (Updated 六月 1, 2015, 7:10 a.m.) Review request for mesos. Bugs: MESOS-2786 https://issues.apache.org/jira/browse/MESOS-2786 Repository: mesos Description --- To compile libprocess on arm64(aarch64) servers, we need to add patches for 3rd software to recognize aarch64 architecture and replace x86 assemble language to standard gcc buildin function Diffs - 3rdparty/libprocess/3rdparty/Makefile.am 519e38c2c22904e75f36b936142a915a8f615b21 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/libev-4.15.patch bbd83e6928e6caba3bc5a9739823d60923cfaa2a 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION Diff: https://reviews.apache.org/r/34881/diff/ Testing --- Build and run mesos-mater/mesos-slave succesly on arm64 server. Thanks, Dong Robin
Re: Review Request 34882: let mesos to compile and run on arm64 servers
On 六月 1, 2015, 10:14 a.m., Mesos ReviewBot wrote: Bad patch! Reviews applied: [34882] Failed command: ./support/apply-review.sh -n -r 34882 Error: 2015-06-01 10:14:22 URL:https://reviews.apache.org/r/34882/diff/raw/ [4278/4278] - 34882.patch [1] 34882.patch:73: trailing whitespace. -lock xadd dword ptr [eax], ecx; 34882.patch:74: trailing whitespace. - lock xadd dword ptr [eax], ebx; 34882.patch:75: trailing whitespace. -mov result, ecx; // result = ebx; 34882.patch:77: trailing whitespace. - return result; warning: 4 lines add whitespace errors. Successfully applied: let mesos to compile and run on arm64 servers To build and run mesos on arm64(aarch64) servers, we add the missing include header file, or it will report: pivot_root is not available Review: https://reviews.apache.org/r/34882 3rdparty/zookeeper-3.4.5.patch:129: trailing whitespace. +-lock xadd dword ptr [eax], ecx; 3rdparty/zookeeper-3.4.5.patch:130: trailing whitespace. +- lock xadd dword ptr [eax], ebx; 3rdparty/zookeeper-3.4.5.patch:131: trailing whitespace. +-mov result, ecx; // result = ebx; 3rdparty/zookeeper-3.4.5.patch:133: trailing whitespace. +- return result; Failed to commit patch I have to add whitespace in patch as the source code in zookeeper*.tgz do have these whitespaces. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34882/#review85963 --- On 六月 1, 2015, 7:11 a.m., Dong Robin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34882/ --- (Updated 六月 1, 2015, 7:11 a.m.) Review request for mesos. Bugs: MESOS-2786 https://issues.apache.org/jira/browse/MESOS-2786 Repository: mesos Description --- To build and run mesos on arm64(aarch64) servers, we add the missing include header file, or it will report: pivot_root is not available Diffs - 3rdparty/leveldb.patch ad8c19b9caa856ff85978ba832d48df11b3a83f0 3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da src/linux/fs.cpp 1c9cf3f2ffead37148e4f6a81cefdbb97f679b09 Diff: https://reviews.apache.org/r/34882/diff/ Testing --- Build and run mesos-master/mesos slave successfully on arm64 server. Thanks, Dong Robin
Re: Review Request 36424: Created a command executor helper method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/#review92232 --- 3rdparty/libprocess/include/process/subprocess.hpp (line 364) https://reviews.apache.org/r/36424/#comment146283 Isn't `++cmdArgs.begin()` the same `cmdArgs.end()` ? I feel that pre-increment looks quite weird. If you still prefer the given style, I think `cmdArgs.begin() + 1` is still more readable and doesn't add complexity. - Alexander Rojas On July 18, 2015, 4 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 18, 2015, 4 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830 and MESOS-2834 factoring out the remote command execution logic. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36612: Cleanup of right angle brackets in cgroups code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36612/#review92235 --- Ship it! Ship It! - Alexander Rojas On July 20, 2015, 10:36 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36612/ --- (Updated July 20, 2015, 10:36 a.m.) Review request for mesos and Timothy Chen. Bugs: Mesos-3086 https://issues.apache.org/jira/browse/Mesos-3086 Repository: mesos Description --- Cleanup of right angle brackets in cgroups code. Diffs - src/linux/cgroups.hpp a47f9a26ead097d5b9a1cf605844d2801d9db6d5 src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e Diff: https://reviews.apache.org/r/36612/diff/ Testing --- make check ubuntu 14.4 Thanks, Joerg Schad
Review Request 36612: Cleanup of right angle brackets in cgroups code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36612/ --- Review request for mesos and Timothy Chen. Bugs: Mesos-3086 https://issues.apache.org/jira/browse/Mesos-3086 Repository: mesos Description --- Cleanup of right angle brackets in cgroups code. Diffs - src/linux/cgroups.hpp a47f9a26ead097d5b9a1cf605844d2801d9db6d5 src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e Diff: https://reviews.apache.org/r/36612/diff/ Testing --- make check ubuntu 14.4 Thanks, Joerg Schad
Re: Review Request 36617: Improved task reconciliation documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36617/ --- (Updated July 20, 2015, 4:21 p.m.) Review request for mesos and Joerg Schad. Repository: mesos Description --- Improved task reconciliation documentation. Diffs (updated) - docs/reconciliation.md 17537ba8420c95d833e64ccf82ff9bb4530497f0 Diff: https://reviews.apache.org/r/36617/diff/ Testing --- https://gist.github.com/nfnt/73532d62fe39d27ff33d Thanks, Jan Schlicht
Review Request 36618: Fixed ROOT_CGROUPS_Tasks and ROOT_CGROUPS_Read on Ubunu 14.04.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36618/ --- Review request for mesos and Benjamin Hindman. Bugs: MESOS-3079 https://issues.apache.org/jira/browse/MESOS-3079 Repository: mesos Description --- See summary. Diffs - src/tests/cgroups_tests.cpp 9f5028f831a5a2fd4f7f0a0b7459c184ea9598fb Diff: https://reviews.apache.org/r/36618/diff/ Testing --- sudo make check Thanks, Artem Harutyunyan
Re: Review Request 34137: Add support for container image provisioners.
On July 17, 2015, 1:36 a.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, line 630 https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line630 Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is called. In general, I think we should avoid using [=] for lambdas because its dangeous! I would prefer we resort to our old style defer style (e.g., introduce `_provision`). Jiang Yan Xu wrote: [=] captures slaveId by value (copy) so it won't be invalid? But after when to use lambdas, I think this is a good point and we should establish some best practices. Google style guide has these guidelines: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions Avoiding default captures is one of them; limiting the length of lambdas is another. We should document these, at least reference Google's. And how about lambdas that simply call another method vs. bind? :) Aha, good call. I checked the C++11 standard, $5.1.2/14 says that capturing a variable of reference type by copy will create a copy of the value referenced instead of creating a copy of the reference. Thus the lambda will have its own copy of the value that the reference was referencing when it was created. However, looks like gcc has a bug related to capturing a reference type by using [=] so we should probably avoid that as much as possible http://stackoverflow.com/questions/6529177/capturing-reference-variable-by-copy-in-c0x-lambda - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92000 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
On July 17, 2015, 1:36 a.m., Jie Yu wrote: src/slave/containerizer/provisioner.cpp, line 24 https://reviews.apache.org/r/34137/diff/3/?file=1009145#file1009145line24 Where is this? This won't compile! You're right it won't, wierd it compiled for me :( let's fix this! - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92000 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36389: Enable remote execution of arbitrary command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/#review92221 --- Apologies for the delay on the review. Let's definitely get a test for this stuff too! src/slave/flags.hpp (lines 118 - 119) https://reviews.apache.org/r/36389/#comment146243 We name the flags to match how you'd see them on the command line and thus use snake case here (see above too). Thanks! Also, I'd like to suggest s/remote_execution_allowed/allow_remote_execution/ in order to be more of a verb here, i.e, --allow_remote_execution=true versus --remote_execution_allowed=true. src/slave/main.cpp (lines 261 - 265) https://reviews.apache.org/r/36389/#comment146245 First, this shoudn't be needed, because the slave will get passed the flags and when it initializes can check to see if remote execution has been allowed. Second, we try and avoid at all costs synchronously calling a libprocess process. There is only one place I know of that does this in some tests, but there is a big TODO and warning of why we don't want to do this. Happy to discuss in more detail. In this case since we shouldn't need this block of code at all you can just kill it. src/slave/slave.hpp (lines 372 - 382) https://reviews.apache.org/r/36389/#comment146246 These shouldn't be needed, the slave will have the flags and can just read the values from there. src/slave/slave.hpp (lines 571 - 575) https://reviews.apache.org/r/36389/#comment146247 Not needed, can just read values from 'flags', no need to have two copies, then on wonders which is the source of truth? This is one of the main reasons we pass 'flags' throughout everywhere as much as possible. src/slave/slave.cpp (line 517) https://reviews.apache.org/r/36389/#comment146250 Do you need to capture 'http'? src/slave/slave.cpp (line 519) https://reviews.apache.org/r/36389/#comment146248 FYI, the 'this-' isn't needed here and we haven't used it consistently. src/slave/slave.cpp (line 4700) https://reviews.apache.org/r/36389/#comment146263 Why `auto` here? Interestingly enough, you do `subprocess.get()` below and pass it to an `OptionSubprocess`, which made me jump for a second thinking that we'd changed the return type of `process::subprocess()` to be `TryOptionprocess::Subprocess` instead of just `Tryprocess::Subprocess`. This is why I'm not convinced `auto` actually buys us anything here: the type provides documentation. src/slave/slave.cpp (line 4701) https://reviews.apache.org/r/36389/#comment146258 What about if 'shell' is true? Seems like we should support both cases, or explicitly return a `BadRequest` or `Unsupported...` if we don't support it for now. src/slave/slave.cpp (lines 4703 - 4705) https://reviews.apache.org/r/36389/#comment146260 One of these things is not like the others. ;-) Any reason you didn't use the 'process::' prefix on the third `Subprocess::PIPE()`? src/slave/slave.cpp (line 4712) https://reviews.apache.org/r/36389/#comment146262 Why is this an `Option`? The return type of `process::subprocess(...)` above is `Tryprocess::Subprocess`, so no need to put it into an option. In fact, why not just use `subprocess.get()` everywhere below instead of `cmd.get()`? src/slave/slave.cpp (line 4713) https://reviews.apache.org/r/36389/#comment146261 If we don't use stderr my recommendation would be to just send it to '/dev/null' by using `process::Subprocess::PATH(/dev/null)` above instead of a `process::Subprocess::PIPE()`. src/slave/slave.cpp (line 4722) https://reviews.apache.org/r/36389/#comment146264 Unused variable. src/slave/slave.cpp (line 4731) https://reviews.apache.org/r/36389/#comment146265 We don't actually know if this exited unless we do `WIFEXITED` first, it might have terminated due to a signal (which we determine via `WIFSIGNALED`). src/slave/slave.cpp (line 4735) https://reviews.apache.org/r/36389/#comment146266 Okay nevermind, it looks like you do use stderr? Either way, calling 'get()' here now is blocking, because technically even though the process is completed we might not yet have read all the data from the pipe. I think what you want here is to wait for all of 'status()', 'out', and 'err', e.g., something like: CHECK_SOME(subprocess.get().out()); CHECK_SOME(subprocess.get().err()); Futurestring out = subprocess.get().out().get(); Futurestring err = subprocess.get().err().get(); return await(subprocess.get().status(), out, err) .then(...); Note that 'await' will just mean the futures have completed, but they might have failed. You'd want 'collect' if you want to make sure they're successful. I think we'd need some overloads to make 'collect' work in this case too, but that's a quick fix, just
Re: Review Request 36389: Enable remote execution of arbitrary command.
On July 20, 2015, 4:20 p.m., Benjamin Hindman wrote: Apologies for the delay on the review. Let's definitely get a test for this stuff too! Ben, not sure if you see my comments. I am not convinced this should be part of Mesos. Should we reach a consensus first before moving this forward? Thanks. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/#review92221 --- On July 14, 2015, 11:20 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/ --- (Updated July 14, 2015, 11:20 p.m.) Review request for mesos, Benjamin Hindman and Cody Maloney. Bugs: MESOS-2830 https://issues.apache.org/jira/browse/MESOS-2830 Repository: mesos Description --- Jira: MESOS-2830 Under certain (maintenance) circumnstance, it may be necessary (or desirable) to execute arbitrary operator's commands on the slave (the entire fleet or a subset thereof) bypassing the Mesos Task execution mechanism; this may typically be necessary for maintenance and/or emergency actions. This patch adds an HTTP endpoint (/execute) which accepts a JSON-encoded CommandInfo structure and executes the given command (with optional arguments). The output of the command (along with potentially any stderr messages) is returned JSON-encoded in the Response. For more details, see the design doc at: https://goo.gl/4npTMU Diffs - src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 Diff: https://reviews.apache.org/r/36389/diff/ Testing --- make check lots of manual testing (using Postman, REST client) Thanks, Marco Massenzio
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- src/tests/fetcher_tests.cpp (line 297) https://reviews.apache.org/r/36501/#comment146333 According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this ``` process::http::URL url( http, process.self().address.ip, process.self().address.port, /help); ``` But I perfer chang it like this ``` const network::Address address = process.self().address; process::http::URL url(http, address.ip, address.port, /help); ``` Or add `using URL` like this ``` using process::Future; using process::http::URL; (Left a blank below and after process::Future) ``` and then ``` const network::Address address = process.self().address; URL url(http, address.ip, address.port, /help); ``` src/tests/fetcher_tests.cpp (line 314) https://reviews.apache.org/r/36501/#comment146334 It would be better to add ```#include stout/stringify.hpp``` after ```#include stout/protobuf.hpp``` - haosdent huang On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 20, 2015, 4:42 p.m., haosdent huang wrote: Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-) - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 34881: let libprocess to compile on arm64 servers
On June 22, 2015, 3:48 a.m., Cody Maloney wrote: 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch, line 18 https://reviews.apache.org/r/34881/diff/1/?file=975553#file975553line18 Where did this header come from? If we were to just update protobuf to a newer version would it just compile? Dong Robin wrote: This header comes from https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/atomicops_internals_generic_gcc.h, the latest protobuf code. If we update the protobuf to the newer version, it would just compile. And, if we update protobuf to the latest version (which already supported arm64 arch), we don't need this patch anymore. feel free to send a patch for https://issues.apache.org/jira/browse/MESOS-2504 to upgrade protobuf to the latest version. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34881/#review88722 --- On June 1, 2015, 7:10 a.m., Dong Robin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34881/ --- (Updated June 1, 2015, 7:10 a.m.) Review request for mesos. Bugs: MESOS-2786 https://issues.apache.org/jira/browse/MESOS-2786 Repository: mesos Description --- To compile libprocess on arm64(aarch64) servers, we need to add patches for 3rd software to recognize aarch64 architecture and replace x86 assemble language to standard gcc buildin function Diffs - 3rdparty/libprocess/3rdparty/Makefile.am 519e38c2c22904e75f36b936142a915a8f615b21 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/libev-4.15.patch bbd83e6928e6caba3bc5a9739823d60923cfaa2a 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION Diff: https://reviews.apache.org/r/34881/diff/ Testing --- Build and run mesos-mater/mesos-slave succesly on arm64 server. Thanks, Dong Robin
Re: Review Request 36402: Adding 'Accept' header in request
On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, line 126 https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126 Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding ) Isabel Jimenez wrote: Added a TODO since it's not standard to return false here. Anand Mazumdar wrote: Looks like I am missing something here. This is a method for Accept-Encoding. If the header is not present and you pass gzip as argument to the method , it should return false as was the case earlier since the client can't accept gzip ( gzip is a bad example here owing to the exception in the rfc around it/compress). Did you get confused with it being the Accept header ? Re-opening the issue for now. I think we can get rid of the TODO here. `Accept` and `Accept-encoding` have the same behavior in this case. From RFC 14.3 Accept-Encoding : If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91843 --- On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 11:54 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36586: Updated scheduler driver to send SUBSCRIBE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/#review92326 --- Ship it! Looks good, just a question around whether we want to be testing for 'FrameworkID' being set correctly, in order to distinguish subscription from re-subscription (yet another time that re-subscription seems to be a useful concept ;)). src/tests/rate_limiting_tests.cpp (lines 134 - 136) https://reviews.apache.org/r/36586/#comment146421 Note that in these tests, we've lost the fact that some of these subscribe calls should not have a FrameworkID (i.e. register) and some should have a FrameworkID (i.e. re-register). I suppose we were technically not testing this before, but at least we had more confidence given the different types of the messages. Should we be adding in some expectations for 'FrameworkID' being set/unset appropriately? - Ben Mahler On July 17, 2015, 11:08 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/ --- (Updated July 17, 2015, 11:08 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp 25e2d660f4ee4c0b21c887f78ad04819012966f9 src/tests/master_tests.cpp 1e934c4b168a0afabd5065e2c8ffa131362ed29b src/tests/rate_limiting_tests.cpp 6a93df086bc0f256c7750d06f950d61f2dfb7b5c src/tests/slave_recovery_tests.cpp de2fc280abae98a5fbbeae6e230a1bfdaf0fc86e Diff: https://reviews.apache.org/r/36586/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/ --- (Updated July 20, 2015, 11 p.m.) Review request for mesos, Adam B, Alexander Rukletsov, Kapil Arya, and Till Toenshoff. Changes --- Till's review. Rebase Bugs: MESOS-2946 https://issues.apache.org/jira/browse/MESOS-2946 Repository: mesos Description --- Splits and updates the original declaration of the `Authorizer` into its interface and a default implementation, the `LocalAuthorizer`. Following the pattern of the modularized `Authenticator`, it generates a default constructor which is required when writing a `TYPED_TEST` in a follow up patch. Additionally, an initialize method has been added, needed for passing in the current ACL definitions as provided via flags. Other changes are just updates to allow for compilation. Diffs (updated) - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/authorizer/authorizer.proto PRE-CREATION include/mesos/mesos.proto cb24125a3f05e0d38fb22e481a15ceb21f882d27 include/mesos/type_utils.hpp 86b37ca1f63e4687af4e86731dceeb1c9c219c5e src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/examples/persistent_volume_framework.cpp c6d6ed337bfca91dc146cb31298cabebdbb13509 src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b src/tests/mesos.hpp 23d9841c7cfb16abef89a53d29a256a2c5e94b52 src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9 Diff: https://reviews.apache.org/r/36048/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36450: Introduced Address and URL protobufs.
On July 19, 2015, 7:38 p.m., Benjamin Hindman wrote: Hasn't this been submitted? Yep, marked as submitted. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review92218 --- On July 17, 2015, 1:36 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto d2f482668e671b30f2586f6aae9c20132ab4d1e4 include/mesos/type_utils.hpp eb7fe2562cfcff52288d1c216425068d1ba551c0 src/common/type_utils.cpp 2ad5b4cbe324c83e81fd7df7430652f5c0a4e30f src/master/master.cpp 082758ef54597ad32cf0d025c147f0f44dd11961 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36514: [MESOS-898] Add CMake-based build system for the process library
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36514/ --- (Updated July 20, 2015, 7:33 p.m.) Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Timothy St. Clair. Changes --- Address issues Artem had. Most of the diff is just moving away from the monolithic `CMakeLists.txt` model and towards modularity. Repository: mesos Description --- This commit is the second in a series of two commits that will introduce a CMake-based build system for Mesos. The first introduced the build system for the core Mesos project; this one will introduce a similar system for the process library. For more details see the core Mesos commit. Diffs (updated) - 3rdparty/libprocess/3rdparty/CMakeLists.txt PRE-CREATION 3rdparty/libprocess/CMakeLists.txt PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake PRE-CREATION 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake PRE-CREATION 3rdparty/libprocess/cmake/macros/External.cmake PRE-CREATION 3rdparty/libprocess/cmake/macros/PatchCommand.cmake PRE-CREATION 3rdparty/libprocess/src/CMakeLists.txt PRE-CREATION 3rdparty/libprocess/src/tests/CMakeLists.txt PRE-CREATION CMakeLists.txt PRE-CREATION cmake/MesosConfigure.cmake PRE-CREATION Diff: https://reviews.apache.org/r/36514/diff/ Testing --- Includes tests for the process library that all run and pass. Thanks, Alex Clemmer
Re: Review Request 36618: Fixed ROOT_CGROUPS_Tasks and ROOT_CGROUPS_Read on Ubunu 14.04.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36618/#review92272 --- src/tests/cgroups_tests.cpp (line 438) https://reviews.apache.org/r/36618/#comment146337 Is invalid actually a valid hierarchy? Just want to know what's the rationale changinig this to invalid42 - Timothy Chen On July 20, 2015, 2:56 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36618/ --- (Updated July 20, 2015, 2:56 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3079 https://issues.apache.org/jira/browse/MESOS-3079 Repository: mesos Description --- See summary. Diffs - src/tests/cgroups_tests.cpp 9f5028f831a5a2fd4f7f0a0b7459c184ea9598fb Diff: https://reviews.apache.org/r/36618/diff/ Testing --- sudo make check Thanks, Artem Harutyunyan
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 21, 2015, 12:58 a.m., Ben Mahler wrote: Thanks Anand! Couple of issues, per our chat on IRC: (1) There is no 'pid' for pure HTTP schedulers, so we'll need to ensure that the master / slave can handle not having a framework pid, which is a bit tricky to get right. Note that the slave directly sends messages to frameworks, so we'll need to make sure that it can differentiate between when it needs to forward to the master vs. directly to the framework. There's also a bunch of code in the master that uses '`framework-pid`' that we'll need to go over and update to handle http schedulers. (2) To continue to support the scheduler driver's message passing optimization while still having a driver that speaks HTTP, we'll need to pass it's PID somehow. Vinod and I were thinking of having a special HTTP header to pass it through. I left some other comments, but let's figure out a plan during the http sync tomorrow. Thanks for the review. We can easily deal with both of the issues i.e. the 'pid of http schedulers'/'scheduler driver message passing' independently in a separate change and should not block this review ? This abstraction for FrameworkDriver already takes care of all the occurences of framework-pid and correctly resolves them to writing the contents on the stream for http schedulers. On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/common/protobuf_utils.hpp, line 83 https://reviews.apache.org/r/36318/diff/3/?file=1012218#file1012218line83 Why define this? Seems that we would want this case to be a compilation error instead of having an Error at runtime. This has to do with how the FrameworkDriver::send is templated. It's a function that handles both events and messages ( to ensure backward compatibility ). The compiler would barf out if I don't have this generic function that handles a generic message. This would anyways go away when we have implemented support for all the message types we support. On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/common/protobuf_utils.cpp, lines 57-60 https://reviews.apache.org/r/36318/diff/3/?file=1012219#file1012219line57 Mind fixing this in a separate patch since it's independent? We can get it committed quickly that way. Also, seems like we should have had message as an Option, but let's save that for later. This is a chicken and a egg problem. I need to access the delcared functions in the header file and as soon as I include that, I would need to remove these default argument values. The only reason this worked till now was that someone accidently forgot including the header file. :) Do you want me to ignore including the header file for now too like others did ? On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/common/protobuf_utils.cpp, lines 193-202 https://reviews.apache.org/r/36318/diff/3/?file=1012219#file1012219line193 Why is this a Try? It seems pretty dangerous for message conversion to fail at run time, was there some errors we need to capture, even if we remove the generic 'Message' parsing error here (move it to compile time error). The only reason for having this as a TryEvent was to make the generic messages that have not been implemented throw an error. Eventually when we have implement all the message types, we can change this to just being events. I can add a TODO for this ? On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/master/http.cpp, lines 314-325 https://reviews.apache.org/r/36318/diff/3/?file=1012220#file1012220line314 Whoops, this will crash if contentType is none. Mind also being explicit about protobuf, rather than assuming that !json implies protobuf? Also, would you mind pulling out this code into a separate patch? It will make it easier to land incrementally :) I had already added a TODO for this // Add validations for Content-Type, Accept headers being present. This is being worked upon by Isabel as part of the validations change. I can add a separate TODO for being more explicit about protobuf ? On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/master/http.cpp, line 346 https://reviews.apache.org/r/36318/diff/3/?file=1012220#file1012220line346 Not yours, but a 501 seems more appropriate here for now? Sure, I can make the change. On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/master/master.cpp, line 1747 https://reviews.apache.org/r/36318/diff/3/?file=101#file101line1747 frameworkInfo.name() is not equivalent to the framework's pid. Per our chat on IRC, we won't have the PID for pure HTTP frameworks, which means that the slave needs to send messages through the master, and we'll probably need to re-work some more of the master code as well. We can easily carry out the refactoring to not need framework pid as part of another change. This change allows us to get the basic set up
Re: Review Request 36402: Adding 'Accept' header in request
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review92343
---
Thanks Isabel!
Could we remove the changes to 'acceptEncoding'? The references to the RFC are
more appropriate in the implementation, rather than the API documentation. In
the API documentation, we can refer people to the RFC, and we may want to take
note of places where we're not following the RFC (e.g. missing
'Accept-Encoding' header and curl). But I originally wrote these comments to
guide someone who is trying to understand the implementation. Also, it's
independent :)
How about using candidates (similarly to 'acceptsEncoding')? That would make
both of these pretty similar and easier to understand. Also note that we should
watch out for the existing bug in 'acceptsEncoding':
```
bool Request::acceptsMediaType(const string mediaType_) const
{
vectorstring mediaType = strings::tokenize(mediaType_, /);
if (mediaType.size() != 2) {
return false;
}
Optionstring accept = headers.get(Accept);
// If no Accept header field is present, then it is assumed
// that the client accepts all media types.
if (accept.isNone()) {
return true;
}
// Remove spaces and tabs for easier parsing.
accept = strings::remove(accept.get(), );
accept = strings::remove(accept.get(), \t);
accept = strings::remove(accept.get(), \n);
// First look for the exact media type, followed by
// a type/* match, followed by */*.
vectorstring candidates;
candidates.push_back(mediaType[0] + / + mediaType[1]);
candidates.push_back(mediaType[0] + /*);
candidates.push_back(*/*);
foreach (const string candidate, candidates) {
// Similar code to 'acceptsEncoding'.
// NOTE that it has a bug because startswith will match
// only by prefix!
}
...
}
```
3rdparty/libprocess/src/http.cpp (lines 122 - 127)
https://reviews.apache.org/r/36402/#comment146454
Actually, thinking over this again, I recall that curl doesn't set
Accept-Encoding by default and does not decompress by default, which is pretty
annoying. We should probably leave a NOTE here about this for now, as to why
this returns false.
Also, s/Accept/Accept-Encoding/
3rdparty/libprocess/src/http.cpp (line 142)
https://reviews.apache.org/r/36402/#comment146471
Only doing startswith was actually a bug on my part! e.g. gzipper;q=1.0
would match gzip..
3rdparty/libprocess/src/http.cpp (line 168)
https://reviews.apache.org/r/36402/#comment146456
What do you mean here by fully support, can you spell out what part is
not supported? It looks like we don't need to:
Note that [HTML20] included an optional level parameter; in practice,
this parameter was never used and has been removed from this specification.
[HTML30] also suggested a version parameter; in practice, this parameter also
was never used and has been removed from this specification.
3rdparty/libprocess/src/http.cpp (line 170)
https://reviews.apache.org/r/36402/#comment146462
s/accepted/accept/
3rdparty/libprocess/src/http.cpp (line 183)
https://reviews.apache.org/r/36402/#comment146464
s/content/accepted/ or s/content/acceptable/
3rdparty/libprocess/src/http.cpp (lines 185 - 187)
https://reviews.apache.org/r/36402/#comment146461
This seems like an malformed 'Accept' header, any reason to prefer
returning false instead of just skipping? Ideally, we've validated already
against malformed requests?
3rdparty/libprocess/src/http.cpp (lines 189 - 190)
https://reviews.apache.org/r/36402/#comment146466
Whoops, don't you need to check the size of this? It looks like valid input
needs 2 tokens? Perhaps we should do this near the beginning?
3rdparty/libprocess/src/http.cpp (lines 190 - 199)
https://reviews.apache.org/r/36402/#comment146465
Let's get some newlines in here to make it more readable :)
3rdparty/libprocess/src/http.cpp (lines 192 - 193)
https://reviews.apache.org/r/36402/#comment146467
Is startswith sufficient? Seems like you need a full match?
Hm.. it doesn't look like `*/foo` should be considered valid, but this
code considers `*/foo` to match `bar/foo`.
3rdparty/libprocess/src/http.cpp (lines 197 - 199)
https://reviews.apache.org/r/36402/#comment146468
Hm.. we should split on ';' before, it's not really part of the content
token.
- Ben Mahler
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/
---
(Updated July 15, 2015, 11:54 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and
Vinod Kone.
Repository: mesos-incubating
Description
---
Adding a
Re: Review Request 36608: Add ppc64le architecture support for leveldb and zookeeper
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36608/#review92364 --- Bad patch! Reviews applied: [36607] Failed command: ./support/apply-review.sh -n -r 36607 Error: 2015-07-21 02:25:57 URL:https://reviews.apache.org/r/36607/diff/raw/ [997/997] - 36607.patch [1] error: missing binary patch data for '3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz' error: binary patch does not apply to '3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz' error: 3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz: patch does not apply error: missing binary patch data for '3rdparty/libprocess/3rdparty/libev-4.15.tar.gz' error: binary patch does not apply to '3rdparty/libprocess/3rdparty/libev-4.15.tar.gz' error: 3rdparty/libprocess/3rdparty/libev-4.15.tar.gz: patch does not apply error: missing binary patch data for '3rdparty/libprocess/3rdparty/protobuf-2.5.0.tar.gz' error: binary patch does not apply to '3rdparty/libprocess/3rdparty/protobuf-2.5.0.tar.gz' error: 3rdparty/libprocess/3rdparty/protobuf-2.5.0.tar.gz: patch does not apply Failed to apply patch - Mesos ReviewBot On July 20, 2015, 3:45 a.m., Jihun Kang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36608/ --- (Updated July 20, 2015, 3:45 a.m.) Review request for mesos. Bugs: MESOS-3084 https://issues.apache.org/jira/browse/MESOS-3084 Repository: mesos Description --- Add ppc64le architecture support for leveldb and zookeeper Diffs - 3rdparty/leveldb.tar.gz b6ea2c7df8f0eef687f9ad90af70f35f81743cbc 3rdparty/zookeeper-3.4.5.tar.gz 1a547fe17a6fad86012f855d3c4cc38fed4899fc Diff: https://reviews.apache.org/r/36608/diff/ Testing --- make check has passed on mesos-0.22.1. [--] Global test environment tear-down [==] 539 tests from 86 test cases ran. (366753 ms total) [ PASSED ] 539 tests. Thanks, Jihun Kang
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/#review92329 --- Thanks Anand! Couple of issues, per our chat on IRC: (1) There is no 'pid' for pure HTTP schedulers, so we'll need to ensure that the master / slave can handle not having a framework pid, which is a bit tricky to get right. Note that the slave directly sends messages to frameworks, so we'll need to make sure that it can differentiate between when it needs to forward to the master vs. directly to the framework. There's also a bunch of code in the master that uses '`framework-pid`' that we'll need to go over and update to handle http schedulers. (2) To continue to support the scheduler driver's message passing optimization while still having a driver that speaks HTTP, we'll need to pass it's PID somehow. Vinod and I were thinking of having a special HTTP header to pass it through. I left some other comments, but let's figure out a plan during the http sync tomorrow. src/common/protobuf_utils.hpp (line 80) https://reviews.apache.org/r/36318/#comment146423 We usually add the argument name in the header, so: ``` Tryscheduler::Event event(const FrameworkRegisteredMessage message); ``` src/common/protobuf_utils.hpp (line 83) https://reviews.apache.org/r/36318/#comment146422 Why define this? Seems that we would want this case to be a compilation error instead of having an Error at runtime. src/common/protobuf_utils.cpp (lines 57 - 60) https://reviews.apache.org/r/36318/#comment146424 Mind fixing this in a separate patch since it's independent? We can get it committed quickly that way. Also, seems like we should have had message as an Option, but let's save that for later. src/common/protobuf_utils.cpp (lines 193 - 202) https://reviews.apache.org/r/36318/#comment146425 Why is this a Try? It seems pretty dangerous for message conversion to fail at run time, was there some errors we need to capture, even if we remove the generic 'Message' parsing error here (move it to compile time error). src/master/http.cpp (lines 314 - 325) https://reviews.apache.org/r/36318/#comment146450 Whoops, this will crash if contentType is none. Mind also being explicit about protobuf, rather than assuming that !json implies protobuf? Also, would you mind pulling out this code into a separate patch? It will make it easier to land incrementally :) src/master/http.cpp (line 346) https://reviews.apache.org/r/36318/#comment146451 Not yours, but a 501 seems more appropriate here for now? src/master/master.cpp (line 1747) https://reviews.apache.org/r/36318/#comment146452 frameworkInfo.name() is not equivalent to the framework's pid. Per our chat on IRC, we won't have the PID for pure HTTP frameworks, which means that the slave needs to send messages through the master, and we'll probably need to re-work some more of the master code as well. - Ben Mahler On July 15, 2015, 4:56 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 15, 2015, 4:56 a.m.) Review request for mesos, Benjamin Hindman, 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,...) - FrameworkDriver can be of 2 types http, libprocess. An Optional member variable is used to distinguiush between them. - 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 9ac81c38efd70f92c64a5865fa79fe516e84dd92 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test
Review Request 36629: stout: Added support for 'synchronized_wait'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36629/ --- Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/synchronized.hpp e40ec55f7818fad8703787ecb67869c9e1922e85 Diff: https://reviews.apache.org/r/36629/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 34137: Add support for container image provisioners.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92339 --- src/slave/containerizer/mesos/containerizer.cpp (lines 185 - 186) https://reviews.apache.org/r/34137/#comment146447 Hum, you put provisioner.cpp under OS_LINUX guard. There'll be a link error on OSX since MesosContainerizer supports all platforms. - Jie Yu On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36610: Add explicit syscall header file to linux fs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36610/#review92367 --- Patch looks great! Reviews applied: [36610] All tests passed. - Mesos ReviewBot On July 20, 2015, 4:21 a.m., Jihun Kang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36610/ --- (Updated July 20, 2015, 4:21 a.m.) Review request for mesos. Bugs: MESOS-3085 https://issues.apache.org/jira/browse/MESOS-3085 Repository: mesos Description --- Add explicit syscall header file to linux fs Diffs - src/linux/fs.cpp ea0891e320154b85a21ed2d138c192821efae9cd Diff: https://reviews.apache.org/r/36610/diff/ Testing --- Thanks, Jihun Kang
Re: Review Request 36514: [MESOS-898] Add CMake-based build system for the process library
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36514/#review92368 --- Bad patch! Reviews applied: [36513] Failed command: ./support/apply-review.sh -n -r 36513 Error: 2015-07-21 03:37:30 URL:https://reviews.apache.org/r/36513/diff/raw/ [5680/5680] - 36513.patch [1] 36513.patch:11: trailing whitespace. # Licensed under the Apache License, Version 2.0 (the License); you 36513.patch:12: trailing whitespace. # may not use this file except in compliance with the License. You may 36513.patch:13: trailing whitespace. # obtain a copy of the License at 36513.patch:15: trailing whitespace. #http://www.apache.org/licenses/LICENSE-2.0 36513.patch:17: trailing whitespace. # Unless required by applicable law or agreed to in writing, software warning: squelched 11 whitespace errors warning: 16 lines add whitespace errors. Successfully applied: [MESOS-898] Add CMake-based build system for Mesos project This is the first of two commits that will introduce a CMake-based build system for the Mesos project. These commits are split along the libprocess boundary -- this first commit will provide the CMake build logic for the core Mesos project, while the second one will introduce the CMake build logic for the process library. They are committed independently to provide a natural history boundary between the two projects. CMake provides a number of advantages over the Make-based build system currently in place. The most obvious of these is much greater platform independence -- CMake supports a wide variety of platforms and environments (even Visual Studio on Windows), which opens Mesos up for contributions from organizations with drastically different toolchains than the current Mesos default. The second is that this gives us an opportunity to audit the existing build system. The plan moving forward is to develop the CMake build system incrementally and in paralllel to the autoconf build system. Review: https://reviews.apache.org/r/36513 CMakeLists.txt:5: trailing whitespace. +# Licensed under the Apache License, Version 2.0 (the License); you CMakeLists.txt:6: trailing whitespace. +# may not use this file except in compliance with the License. You may CMakeLists.txt:7: trailing whitespace. +# obtain a copy of the License at CMakeLists.txt:9: trailing whitespace. +#http://www.apache.org/licenses/LICENSE-2.0 CMakeLists.txt:11: trailing whitespace. +# Unless required by applicable law or agreed to in writing, software CMakeLists.txt:12: trailing whitespace. +# distributed under the License is distributed on an AS IS BASIS, CMakeLists.txt:14: trailing whitespace. +# See the License for the specific language governing permissions and CMakeLists.txt:15: trailing whitespace. +# limitations under the License. cmake/MesosConfigure.cmake:5: trailing whitespace. +# Licensed under the Apache License, Version 2.0 (the License); you cmake/MesosConfigure.cmake:6: trailing whitespace. +# may not use this file except in compliance with the License. You may cmake/MesosConfigure.cmake:7: trailing whitespace. +# obtain a copy of the License at cmake/MesosConfigure.cmake:9: trailing whitespace. +#http://www.apache.org/licenses/LICENSE-2.0 cmake/MesosConfigure.cmake:11: trailing whitespace. +# Unless required by applicable law or agreed to in writing, software cmake/MesosConfigure.cmake:12: trailing whitespace. +# distributed under the License is distributed on an AS IS BASIS, cmake/MesosConfigure.cmake:14: trailing whitespace. +# See the License for the specific language governing permissions and cmake/MesosConfigure.cmake:15: trailing whitespace. +# limitations under the License. Failed to commit patch - Mesos ReviewBot On July 20, 2015, 7:33 p.m., Alex Clemmer wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36514/ --- (Updated July 20, 2015, 7:33 p.m.) Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Timothy St. Clair. Repository: mesos Description --- This commit is the second in a series of two commits that will introduce a CMake-based build system for Mesos. The first introduced the build system for the core Mesos project; this one will introduce a similar system for the process library. For more details see the core Mesos commit. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt PRE-CREATION 3rdparty/libprocess/CMakeLists.txt PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake PRE-CREATION 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake PRE-CREATION 3rdparty/libprocess/cmake/macros/External.cmake PRE-CREATION 3rdparty/libprocess/cmake/macros/PatchCommand.cmake
Re: Review Request 36629: stout: Added support for 'synchronized_wait'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36629/#review92371 --- Patch looks great! Reviews applied: [36629] All tests passed. - Mesos ReviewBot On July 21, 2015, 1:11 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36629/ --- (Updated July 21, 2015, 1:11 a.m.) Review request for mesos, Benjamin Hindman and Joris Van Remoortere. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/synchronized.hpp e40ec55f7818fad8703787ecb67869c9e1922e85 Diff: https://reviews.apache.org/r/36629/diff/ Testing --- `make check` Thanks, Michael Park
