Re: Review Request 37192: More basic call validation tests for http api
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/ --- (Updated Aug. 11, 2015, 10:58 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Change endpoint names and rebased Repository: mesos Description --- Added more basic call validation tests around malformed body, invalid media types etc for the http api Diffs (updated) - src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37192/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 10:57 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Change endpoint name in master to api/v1/scheduler Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 36979: Updating all references to os::shell
On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
src/hdfs/hdfs.hpp, lines 123-127
https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line123
Wait, how was `|| true` the existing semantics? We are definitely
capturing stderr into stdout, but I don't see anything else here unless I'm
missing something?
You are actually right - adding || true alters the semantics - removed.
However, please note that, as we discussed, once the shell command exits with a
non-zero error code, we just Error() out, and do not return stdout (or stderr,
for that matter).
The error message (stderr piped to stdout) will be in the logs emitted by
`os::shell()`
On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
src/hdfs/hdfs.hpp, line 159
https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line159
This is a really pesky nit, but in the above functions you decided to
call the variable capturing the result of `os::shell` to be called `out`,
but here and below you decided `output`? Let's pick one and be consistent
per this file please.
yes, sorry, I was re-using existing variables as far as I could and, as they
are local variables, I didn't consider that.
Done.
On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
src/hdfs/hdfs.hpp, lines 69-88
https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line69
Let's actually capture the error message because when debugging it'll
be super nice to see why it failed in the logs instead of nothing (so kill
the simplification comment and if you're concerned that someone will just
come around and simplify it later leave a comment why we're capturing
`out.error()`.
Then let's just return `true` if not an error to keep with previous
semantics (I don't know the details of `hadoop version` to suggest
otherwise, so no need to stray).
LOL - so I did some further analysis and the original code actually was NOT
doing what it thought it did (I think - difficult to say: no docs).
By passing NULL as the out in os::shell() it was not getting anything: looking
at shell.hpp#L56 (`if (os != NULL) { ... }`) - adding the 21 was a nice touch
:)
Anyways, I did as you suggested, but I'm afraid the error message won't be that
much helpful (apart from stating that `hadoop version` failed with exit code
xx).
BUT, I quite like your suggestion, so I've added a `LOG(ERROR) stdout` in
os::shell() if the exit code != 0.
(please let me know if that's overkill, though!)
On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
src/tests/containerizer/port_mapping_tests.cpp, line 975
https://reviews.apache.org/r/36979/diff/3/?file=1033651#file1033651line975
Minor nit, how about here and below:
ASSERT_FALSE(strings::contains(invalid.error(), 256));
Benjamin Hindman wrote:
Also, do these need to be ASSERT? I know you're just inheriting bad code
here, but might as well clean it up.
Done (and it was ASSERT_TRUE() but I got the point :)
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36979/#review94880
---
On Aug. 6, 2015, 6:24 p.m., Marco Massenzio wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36979/
---
(Updated Aug. 6, 2015, 6:24 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Bugs: MESOS-3142
https://issues.apache.org/jira/browse/MESOS-3142
Repository: mesos
Description
---
Updating all references to os::shell
For more details see MESOS-3142.
Diffs
-
src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7
src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce
src/slave/containerizer/isolators/network/port_mapping.cpp
8244c345b84108af7fa18d20e71401d6e1a0aeb0
src/tests/containerizer/isolator_tests.cpp
ff6e2b7e190a58a4809d6e71addb15dabe418e17
src/tests/containerizer/port_mapping_tests.cpp
4bee74acba2b1472c80cabbc9d0384bd04c543aa
Diff: https://reviews.apache.org/r/36979/diff/
Testing
---
make check
*Note*: this patch fixes breakages introduce by the refactoring in:
https://reviews.apache.org/r/36978
Thanks,
Marco Massenzio
Re: Review Request 37189: Added std::hash template specializations.
On Aug. 11, 2015, 5:39 a.m., Michael Park wrote: Looks good overall! Same question as [r37188](https://reviews.apache.org/r/37188/): why did you decide to leave the `hash_value` functions and call it from `std::hash` specializations rather than moving the logic? I assumed that there may be other functionality that depends on the `hash_value` functions. After moving the logic to `std::hash`, the only dependencies were in stout/cache.hpp and stout/multihashmap.hpp. Both also relied on boost, which I changed to std includes. See r37187 for the added changes. - Jan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37189/#review94859 --- On Aug. 11, 2015, 11:58 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37189/ --- (Updated Aug. 11, 2015, 11:58 a.m.) Review request for mesos, Alexander Rojas and Michael Park. Bugs: MESOS-3217 https://issues.apache.org/jira/browse/MESOS-3217 Repository: mesos Description --- Added std::hash template specializations. Diffs - include/mesos/type_utils.hpp 6ff061c94e9d3dbb55bd047840b2012d11e3a0f2 src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 src/tests/module.hpp 03756a2536ca3e662ba422e96d121a6c39bb8c84 Diff: https://reviews.apache.org/r/37189/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 37228: Updated slave task label decorator hook to pass in ExecutorInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37228/#review94894 --- Ship it! Ship It! - Till Toenshoff On Aug. 7, 2015, 10:44 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37228/ --- (Updated Aug. 7, 2015, 10:44 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-3236 https://issues.apache.org/jira/browse/MESOS-3236 Repository: mesos Description --- If the task being launched has a command executor, there is no way for the hook to determine the executor-id for that task. This update calls the hook _after_ the ExecutorInfo has been created and thus is able to pass in ExecutorInfo to the label decorator. Diffs - include/mesos/hook.hpp bb5a635dcf189e1023f1eec66fc06955f816fc0b src/examples/test_hook_module.cpp c664b565bcf18dd2153205990119cc679e4ad6cf src/hook/manager.hpp 8153ce4826f94d5771c93d37c59fdc4991352e66 src/hook/manager.cpp 11e6b0a2c0df1d0d7039aaad94e1c6f0e5cc6bc2 src/slave/slave.cpp f181b1b23cec57a9cce6311127f733f17fbd87e4 Diff: https://reviews.apache.org/r/37228/diff/ Testing --- make check. Thanks, Kapil Arya
Re: Review Request 37273: [2/3]Add CMake macro VsBuildCommand in libprocess.
On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/glog-0.3.3.patch, line 21 https://reviews.apache.org/r/37273/diff/2/?file=1036048#file1036048line21 Looks like your updating the patchfile here to include my glog PR that opens it to working on MSVC 1900? I actually have an open review for this, here[1], but I take a different approach. I'd be grateful for your feedback, but it's a bit stale -- now that there's an official patch in the works, we can just tell the Windows port to point at the version of glog that includes that patch. Alex Clemmer wrote: Sorry, I forgot to add the link itself: [1] https://reviews.apache.org/r/37020/ [2] https://reviews.apache.org/r/37021/ So we no need patch glog now. :-) - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/#review94776 --- On Aug. 10, 2015, 9:50 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/ --- (Updated Aug. 10, 2015, 9:50 a.m.) Review request for mesos and Alex Clemmer. Repository: mesos Description --- Add CMake macro VsBuildCommand in libprocess. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION Diff: https://reviews.apache.org/r/37273/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37187: Use std::unordered_{set, map} instead of boost::unordered_{set, map}.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37187/
---
(Updated Aug. 11, 2015, 11:56 a.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
---
Review fixes, change cache.hpp and multihashmap.hpp as well.
Summary (updated)
-
Use std::unordered_{set,map} instead of boost::unordered_{set,map}.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
---
Changed hashmap, hashset to use std::unordered_{set,map} instead of
boost::unordered_{set,map}.
Diffs (updated)
-
3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp
25684a405bfa9c4ab65641568341652a8efaf925
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp
ecab60a21765c58b0732de747509aa6382d31c06
3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp
2dd1905a4626a7d7e9f61863c8290ae1cdb9b925
3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp
a8573ed67e20b5206afd69bab4f5dc094a7e882f
3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp
e8ebe0b2f5e49657ee191a2535e0abdaf8e665ce
3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp
3802a29b82da57217dd75c6b1611fd21c91cfc03
Diff: https://reviews.apache.org/r/37187/diff/
Testing
---
make check
Thanks,
Jan Schlicht
Re: Review Request 36979: Updating all references to os::shell
On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote: src/tests/containerizer/port_mapping_tests.cpp, line 975 https://reviews.apache.org/r/36979/diff/3/?file=1033651#file1033651line975 Minor nit, how about here and below: ASSERT_FALSE(strings::contains(invalid.error(), 256)); Also, do these need to be ASSERT? I know you're just inheriting bad code here, but might as well clean it up. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94880 --- On Aug. 6, 2015, 6:24 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 6, 2015, 6:24 p.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Updating all references to os::shell For more details see MESOS-3142. Diffs - src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7 src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce src/slave/containerizer/isolators/network/port_mapping.cpp 8244c345b84108af7fa18d20e71401d6e1a0aeb0 src/tests/containerizer/isolator_tests.cpp ff6e2b7e190a58a4809d6e71addb15dabe418e17 src/tests/containerizer/port_mapping_tests.cpp 4bee74acba2b1472c80cabbc9d0384bd04c543aa Diff: https://reviews.apache.org/r/36979/diff/ Testing --- make check *Note*: this patch fixes breakages introduce by the refactoring in: https://reviews.apache.org/r/36978 Thanks, Marco Massenzio
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 11, 2015, 7:36 a.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Changes --- Replaced the test check; added a LOG(ERROR) stdout for when the command errors out. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Refactoring os::shell. See MESOS-3142 for more details. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 4b7a7bafe1c64183d021b39cf12523250491f0ee 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1 Diff: https://reviews.apache.org/r/36978/diff/ Testing --- make check *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also https://reviews.apache.org/r/36979/ Thanks, Marco Massenzio
Re: Review Request 37289: Corrected the comments for DRFSorter::dirty.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37289/ --- (Updated Aug. 11, 2015, 9:34 a.m.) Review request for mesos. Bugs: MESOS-3245 https://issues.apache.org/jira/browse/MESOS-3245 Repository: mesos Description --- Corrected the comments for DRFSorter::dirty. Diffs (updated) - src/master/allocator/sorter/drf/sorter.hpp f66ade06c6a5b4bf816839477cec2d18036c7b1a Diff: https://reviews.apache.org/r/37289/diff/ Testing --- Thanks, Qian Zhang
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 11, 2015, 7:37 a.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Changes --- Ben's comments. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Updating all references to os::shell For more details see MESOS-3142. Diffs (updated) - src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7 src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce src/slave/containerizer/isolators/network/port_mapping.cpp 88c0cbc61f3f97b084cc3b3fae8999b07d4aa1c7 src/tests/containerizer/isolator_tests.cpp dd1ae22865ce4467da5ed819e7f0a1cbb834371d src/tests/containerizer/port_mapping_tests.cpp 3c9b7c816a03e2994a353674c5963f1dda043124 Diff: https://reviews.apache.org/r/36979/diff/ Testing --- make check *Note*: this patch fixes breakages introduce by the refactoring in: https://reviews.apache.org/r/36978 Thanks, Marco Massenzio
Re: Review Request 37273: [2/3]Add CMake macro VsBuildCommand in libprocess.
On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 55 https://reviews.apache.org/r/37273/diff/2/?file=1036047#file1036047line55 Sorry, maybe I'm a bit slow this morning -- but how are you running this? Windows doesn't have the `patch` utility, right? How does this actually get patched on Windows? Oh, I have installed MinGW. So the we move it to github and download? On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 77 https://reviews.apache.org/r/37273/diff/2/?file=1036047#file1036047line77 So, why change the value to `TRUE` here? Is there some consequence of this, or is it just clearer to you? (Also, if we want to go this way, it probably makes sense to change the `` we use in the `ExternalProject_Add` calls as well, just for consistency.) Hmm, because I find ``` set(GLOG_CONFIG_CMD ) ``` the visual studio would still execute default configure step which requires CMakeLists.txt . I still don't know why its behaivour would become this here. On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat, line 1 https://reviews.apache.org/r/37273/diff/2/?file=1036052#file1036052line1 In glog's solution, you're required to migrate the project to VS2015 before building it. Does this script do that automatically? (Sorry about the noob question, I don't actually know how this works.) I check and update the solution file here. ``` FINDSTR ^ /C:Microsoft Visual Studio Solution File, Format Version %SOLUTION_VER% ^ %SOLUTION_FILE:/=\% if %errorlevel% neq 0 ( devenv /upgrade %SOLUTION_FILE% ) ``` - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/#review94776 --- On Aug. 10, 2015, 9:50 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/ --- (Updated Aug. 10, 2015, 9:50 a.m.) Review request for mesos and Alex Clemmer. Repository: mesos Description --- Add CMake macro VsBuildCommand in libprocess. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION Diff: https://reviews.apache.org/r/37273/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37188: Added std::hash template specializations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37188/ --- (Updated Aug. 11, 2015, 11:57 a.m.) Review request for mesos, Alexander Rojas and Michael Park. Changes --- Remove hash_value functions. Bugs: MESOS-3217 https://issues.apache.org/jira/browse/MESOS-3217 Repository: mesos Description --- Added std::hash template specializations. Diffs (updated) - 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3 Diff: https://reviews.apache.org/r/37188/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 37189: Added std::hash template specializations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37189/ --- (Updated Aug. 11, 2015, 11:58 a.m.) Review request for mesos, Alexander Rojas and Michael Park. Changes --- Remove hash_value functions, fix review issues. Bugs: MESOS-3217 https://issues.apache.org/jira/browse/MESOS-3217 Repository: mesos Description --- Added std::hash template specializations. Diffs (updated) - include/mesos/type_utils.hpp 6ff061c94e9d3dbb55bd047840b2012d11e3a0f2 src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 src/tests/module.hpp 03756a2536ca3e662ba422e96d121a6c39bb8c84 Diff: https://reviews.apache.org/r/37189/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/#review94881 --- Ship it! 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 53) https://reviews.apache.org/r/36978/#comment149555 And the next iteration would be to use variadic templates and then call strings::format versus strings::internal::format which would make it so that we wouldn't have to do `.c_str()` at all the existing and future call sites! 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (lines 963 - 964) https://reviews.apache.org/r/36978/#comment149556 EXPECT_TRUE(strings::contains(result.get(), No such file or directory)); - Benjamin Hindman On Aug. 6, 2015, 6:20 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 6, 2015, 6:20 p.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Refactoring os::shell. See MESOS-3142 for more details. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 4b7a7bafe1c64183d021b39cf12523250491f0ee 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1 Diff: https://reviews.apache.org/r/36978/diff/ Testing --- make check *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also https://reviews.apache.org/r/36979/ Thanks, Marco Massenzio
Re: Review Request 37289: Corrected the comments for DRFSorter::dirty.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37289/#review94903 --- Patch looks great! Reviews applied: [37289] All tests passed. - Mesos ReviewBot On Aug. 11, 2015, 9:34 a.m., Qian Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37289/ --- (Updated Aug. 11, 2015, 9:34 a.m.) Review request for mesos. Bugs: MESOS-3245 https://issues.apache.org/jira/browse/MESOS-3245 Repository: mesos Description --- Corrected the comments for DRFSorter::dirty. Diffs - src/master/allocator/sorter/drf/sorter.hpp f66ade06c6a5b4bf816839477cec2d18036c7b1a Diff: https://reviews.apache.org/r/37289/diff/ Testing --- Thanks, Qian Zhang
Re: Review Request 37189: Added std::hash template specializations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37189/ --- (Updated Aug. 11, 2015, 2:30 p.m.) Review request for mesos, Alexander Rojas and Michael Park. Bugs: MESOS-3217 https://issues.apache.org/jira/browse/MESOS-3217 Repository: mesos Description --- Added std::hash template specializations. Diffs (updated) - include/mesos/type_utils.hpp 6ff061c94e9d3dbb55bd047840b2012d11e3a0f2 src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 src/tests/module.hpp 03756a2536ca3e662ba422e96d121a6c39bb8c84 Diff: https://reviews.apache.org/r/37189/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 37187: Use std::unordered_{set, map} instead of boost::unordered_{set, map}.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37187/
---
(Updated Aug. 11, 2015, 1:57 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
---
Fix failing multihashmap test (hash ordering may differ between STL
implementations).
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
---
Changed hashmap, hashset to use std::unordered_{set,map} instead of
boost::unordered_{set,map}.
Diffs (updated)
-
3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp
25684a405bfa9c4ab65641568341652a8efaf925
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp
ecab60a21765c58b0732de747509aa6382d31c06
3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp
2dd1905a4626a7d7e9f61863c8290ae1cdb9b925
3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp
a8573ed67e20b5206afd69bab4f5dc094a7e882f
3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp
e8ebe0b2f5e49657ee191a2535e0abdaf8e665ce
3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp
3802a29b82da57217dd75c6b1611fd21c91cfc03
3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp
b625ffaeb3672f58fbd9558a868f87404e659c53
Diff: https://reviews.apache.org/r/37187/diff/
Testing
---
make check
Thanks,
Jan Schlicht
Re: Review Request 37228: Updated slave task label decorator hook to pass in ExecutorInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37228/#review94913 --- Ship it! Ship It! - Guangya Liu On 八月 7, 2015, 10:44 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37228/ --- (Updated 八月 7, 2015, 10:44 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-3236 https://issues.apache.org/jira/browse/MESOS-3236 Repository: mesos Description --- If the task being launched has a command executor, there is no way for the hook to determine the executor-id for that task. This update calls the hook _after_ the ExecutorInfo has been created and thus is able to pass in ExecutorInfo to the label decorator. Diffs - include/mesos/hook.hpp bb5a635dcf189e1023f1eec66fc06955f816fc0b src/examples/test_hook_module.cpp c664b565bcf18dd2153205990119cc679e4ad6cf src/hook/manager.hpp 8153ce4826f94d5771c93d37c59fdc4991352e66 src/hook/manager.cpp 11e6b0a2c0df1d0d7039aaad94e1c6f0e5cc6bc2 src/slave/slave.cpp f181b1b23cec57a9cce6311127f733f17fbd87e4 Diff: https://reviews.apache.org/r/37228/diff/ Testing --- make check. Thanks, Kapil Arya
Re: Review Request 37189: Added std::hash template specializations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37189/#review94912 --- Patch looks great! Reviews applied: [37187, 37188, 37189] All tests passed. - Mesos ReviewBot On Aug. 11, 2015, 12:30 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37189/ --- (Updated Aug. 11, 2015, 12:30 p.m.) Review request for mesos, Alexander Rojas and Michael Park. Bugs: MESOS-3217 https://issues.apache.org/jira/browse/MESOS-3217 Repository: mesos Description --- Added std::hash template specializations. Diffs - include/mesos/type_utils.hpp 6ff061c94e9d3dbb55bd047840b2012d11e3a0f2 src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 src/tests/module.hpp 03756a2536ca3e662ba422e96d121a6c39bb8c84 Diff: https://reviews.apache.org/r/37189/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 36429: Add filesystem/linux isolator for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/#review94920 --- Wouldn't it be good if we could have some comments as to how this class is supposed to be used, what does it encapsulate, etc.? At the very least a URL to a design doc or something? Also, all the methods are completely undocumented: this means that, whenever anyone will want in future to use it, they will have to go hunt for the cpp file and reverse engineer the code (with a large amount of guessing as to the intent for the ambiguous parts) trying to figure out what each of them does. (not to mention that it'll be anyone's guess what the methods' arguments are supposed to be). I'm sure that for those 2-3 people who have spent the last year or so thinking about FSIsolators this is all obvious; but not so for those who haven't, and even less so for those poor souls who may want to join the project in future... - Marco Massenzio On July 12, 2015, 4:46 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/ --- (Updated July 12, 2015, 4:46 a.m.) Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved filesystem/linux from review https://reviews.apache.org/r/34135/ Diffs - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/36429/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36429: Add filesystem/linux isolator for persistent volumes.
On Aug. 11, 2015, 3:57 p.m., Marco Massenzio wrote: Wouldn't it be good if we could have some comments as to how this class is supposed to be used, what does it encapsulate, etc.? At the very least a URL to a design doc or something? Also, all the methods are completely undocumented: this means that, whenever anyone will want in future to use it, they will have to go hunt for the cpp file and reverse engineer the code (with a large amount of guessing as to the intent for the ambiguous parts) trying to figure out what each of them does. (not to mention that it'll be anyone's guess what the methods' arguments are supposed to be). I'm sure that for those 2-3 people who have spent the last year or so thinking about FSIsolators this is all obvious; but not so for those who haven't, and even less so for those poor souls who may want to join the project in future... FYI, this patch is being moved to as Ian is on vacation. https://reviews.apache.org/r/37236/ https://reviews.apache.org/r/37330/ - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/#review94920 --- On July 12, 2015, 4:46 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/ --- (Updated July 12, 2015, 4:46 a.m.) Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved filesystem/linux from review https://reviews.apache.org/r/34135/ Diffs - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/36429/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36429: Add filesystem/linux isolator for persistent volumes.
On July 29, 2015, 4:04 p.m., James DeFelice wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 238 https://reviews.apache.org/r/36429/diff/1/?file=1009137#file1009137line238 why MS_SHARED (bidirectional) vs MS_SLAVE (one-way)? MS_SLAVE would probably give better isolation to the host mount-ns. MS_SHARED would probably be better for a use case that I have in mind (doc'd in MESOS-349), especially since cleanup() here does GC on mount points that are children of the sandbox. SHARED mounts if mainly for propagating persistent volumes (imaging the executor has started and a new task with persistent volumes coming in). The sandbox mount will be shared in host mnt namespace and slave in container mnt namespace. FYI, this patch is being moved to as Ian is on vacation. https://reviews.apache.org/r/37236/ https://reviews.apache.org/r/37330/ - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/#review93457 --- On July 12, 2015, 4:46 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/ --- (Updated July 12, 2015, 4:46 a.m.) Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved filesystem/linux from review https://reviews.apache.org/r/34135/ Diffs - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/36429/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 35983: Added /unreserve HTTP endpoint to the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35983/#review90790 --- Ship it! LGTM! - Jie Yu On Aug. 5, 2015, 7:12 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35983/ --- (Updated Aug. 5, 2015, 7:12 p.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2600 https://issues.apache.org/jira/browse/MESOS-2600 Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3 src/master/master.hpp e44174976aa64176916827bec4c911333c9a91db src/master/master.cpp 50b98248463fc4cd48962890c14c7ad64f2b6f43 src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 src/tests/master_validation_tests.cpp 3513bca6fd6773f712d1a647b1757766dc34f948 Diff: https://reviews.apache.org/r/35983/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94941 --- Looking. Minor issues. Please make sure when you fix an issue, you fix it in all the tests. src/tests/http_api_tests.cpp (lines 72 - 93) https://reviews.apache.org/r/37082/#comment149653 Can you add a TODO to move these out into common helpers? src/tests/http_api_tests.cpp (lines 141 - 143) https://reviews.apache.org/r/37082/#comment149655 Instead of repeating this comment and these 2 lines in every test, add a CreaterMasterFlags() overload to the fixture. src/tests/http_api_tests.cpp (line 178) https://reviews.apache.org/r/37082/#comment149654 do you need the temp variable here? here and subsequent tests. src/tests/http_api_tests.cpp (lines 199 - 200) https://reviews.apache.org/r/37082/#comment149667 ditto. src/tests/http_api_tests.cpp (line 221) https://reviews.apache.org/r/37082/#comment149656 s/subcribedId/frameworkId/ src/tests/http_api_tests.cpp (line 243) https://reviews.apache.org/r/37082/#comment149657 kill this. ASSERT_SOME() guarantees that it is not an error. src/tests/http_api_tests.cpp (line 245) https://reviews.apache.org/r/37082/#comment149662 ditto. src/tests/http_api_tests.cpp (lines 248 - 249) https://reviews.apache.org/r/37082/#comment149658 swap the order of the arguments. when writing ASSERT, EXPECT the convention is that the first argument should be the expected value and the second one is the actual value. this is because of the way gmock prints the error message. please fix this here and everywhere else in this file. src/tests/http_api_tests.cpp (line 275) https://reviews.apache.org/r/37082/#comment149660 kill this. src/tests/http_api_tests.cpp (lines 279 - 280) https://reviews.apache.org/r/37082/#comment149661 ditto. src/tests/http_api_tests.cpp (line 287) https://reviews.apache.org/r/37082/#comment149664 s/pid/PID/ src/tests/http_api_tests.cpp (line 288) https://reviews.apache.org/r/37082/#comment149666 s/http/HTTP/ src/tests/http_api_tests.cpp (lines 292 - 293) https://reviews.apache.org/r/37082/#comment149668 ditto. src/tests/http_api_tests.cpp (line 310) https://reviews.apache.org/r/37082/#comment149669 s/http/HTTP/ src/tests/http_api_tests.cpp (line 322) https://reviews.apache.org/r/37082/#comment149670 s/a/an/ src/tests/http_api_tests.cpp (line 357) https://reviews.apache.org/r/37082/#comment149671 ditto. src/tests/http_api_tests.cpp (lines 366 - 367) https://reviews.apache.org/r/37082/#comment149672 ditto. src/tests/http_api_tests.cpp (line 406) https://reviews.apache.org/r/37082/#comment149673 s/'force'/the 'force'/ - Vinod Kone On Aug. 11, 2015, 5:03 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 5:03 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/master/master.hpp 10cc100b1cf10aa7a8a9979a2562a9c5a9e55bd6 src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37277: (WIP) Added Heartbeater to master to send periodic heartbeats to HTTP schedulers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37277/#review94843 --- Looks pretty good, just some minor things. src/master/master.hpp (line 1288) https://reviews.apache.org/r/37277/#comment149502 ? src/master/master.hpp (line 1290) https://reviews.apache.org/r/37277/#comment149501 Why is this an Option? Seems like it is required for this. src/master/master.hpp (line 1302) https://reviews.apache.org/r/37277/#comment149674 Might want to add the 'override' keyword now that we have C++11, the compiler will make sure that we're actually overriding :) src/master/master.hpp (lines 1606 - 1622) https://reviews.apache.org/r/37277/#comment149678 Shouldn't we start the heartbeater once we set the http connection? Otherwise we're starting one with a None connection. That would avoid the need for the CHECK_SOME / dispatch to `update` as well. We should probably just remove update, and do stop/start instead. src/master/master.hpp (line 1630) https://reviews.apache.org/r/37277/#comment149679 Response header or SUBSCRIBED? src/master/master.hpp (lines 1715 - 1716) https://reviews.apache.org/r/37277/#comment149680 Any reason to avoid Owned? - Ben Mahler On Aug. 9, 2015, 8:31 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37277/ --- (Updated Aug. 9, 2015, 8:31 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Ben Mahler. Bugs: MESOS-3131 https://issues.apache.org/jira/browse/MESOS-3131 Repository: mesos Description --- Just wanted to send out the abstraction for feedback. Diffs - src/internal/evolve.hpp 2e0355960c8c771f28f3ed4428cc047e5787fff7 src/internal/evolve.cpp 4678d67c8324e5c15188b5454e7cc6165d22d9bc src/master/master.hpp 28356e4ca24312b8be0138a34805b3d9035a99a3 Diff: https://reviews.apache.org/r/37277/diff/ Testing --- make check No new tests have been added yet. Thanks, Vinod Kone
Re: Review Request 37273: [2/3]Add CMake macro VsBuildCommand in libprocess.
On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 55 https://reviews.apache.org/r/37273/diff/2/?file=1036047#file1036047line55 Sorry, maybe I'm a bit slow this morning -- but how are you running this? Windows doesn't have the `patch` utility, right? How does this actually get patched on Windows? haosdent huang wrote: Oh, I have installed MinGW. So the we move it to github and download? Alex Clemmer wrote: Ah. We can't have a MinGW dependency at all, unfortunately. Could you please consider rebasing this against my recent glog changes, where we download the tarball from GitHub? Got it. Let me change download from github. On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 77 https://reviews.apache.org/r/37273/diff/2/?file=1036047#file1036047line77 So, why change the value to `TRUE` here? Is there some consequence of this, or is it just clearer to you? (Also, if we want to go this way, it probably makes sense to change the `` we use in the `ExternalProject_Add` calls as well, just for consistency.) haosdent huang wrote: Hmm, because I find ``` set(GLOG_CONFIG_CMD ) ``` the visual studio would still execute default configure step which requires CMakeLists.txt . I still don't know why its behaivour would become this here. Alex Clemmer wrote: Interesting! What does it say on your machine. On my machine (I will have to confirm this in a minute) I remember it saying that there is no configure step for glog. Do you see something different? LoL, I suck on this strange point in serveral hours and try possible ways to skip it. But all of them are failed. If I direct pass to ExternalProject_Add, it could skip the configuration step. But I set to a variable and pass this variable as configuration step in ExternalProject_Add. It would become try to use default CMake configuration step, which need CMakeLists.txt. I use message to debug the variable and compare the variables with , I am sure the variable is setted to empty string. My CMake version is 3.3; Visual studio version is 2015 community version; OS is win7; Project code is in a independent disk. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/#review94776 --- On Aug. 10, 2015, 9:50 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/ --- (Updated Aug. 10, 2015, 9:50 a.m.) Review request for mesos and Alex Clemmer. Repository: mesos Description --- Add CMake macro VsBuildCommand in libprocess. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION Diff: https://reviews.apache.org/r/37273/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37273: [2/3]Add CMake macro VsBuildCommand in libprocess.
On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 77 https://reviews.apache.org/r/37273/diff/2/?file=1036047#file1036047line77 So, why change the value to `TRUE` here? Is there some consequence of this, or is it just clearer to you? (Also, if we want to go this way, it probably makes sense to change the `` we use in the `ExternalProject_Add` calls as well, just for consistency.) haosdent huang wrote: Hmm, because I find ``` set(GLOG_CONFIG_CMD ) ``` the visual studio would still execute default configure step which requires CMakeLists.txt . I still don't know why its behaivour would become this here. Alex Clemmer wrote: Interesting! What does it say on your machine. On my machine (I will have to confirm this in a minute) I remember it saying that there is no configure step for glog. Do you see something different? haosdent huang wrote: LoL, I suck on this strange point in serveral hours and try possible ways to skip it. But all of them are failed. If I direct pass to ExternalProject_Add, it could skip the configuration step. But I set to a variable and pass this variable as configuration step in ExternalProject_Add. It would become try to use default CMake configuration step, which need CMakeLists.txt. I use message to debug the variable and compare the variables with , I am sure the variable is setted to empty string. My CMake version is 3.3; Visual studio version is 2015 community version; OS is win7; Project code is in a independent disk. I still try to find the reason recently. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/#review94776 --- On Aug. 10, 2015, 9:50 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/ --- (Updated Aug. 10, 2015, 9:50 a.m.) Review request for mesos and Alex Clemmer. Repository: mesos Description --- Add CMake macro VsBuildCommand in libprocess. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION Diff: https://reviews.apache.org/r/37273/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37192: More basic call validation tests for http api
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/#review94946 --- Ship it! Ship It! - Vinod Kone On Aug. 11, 2015, 5:04 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/ --- (Updated Aug. 11, 2015, 5:04 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Added more basic call validation tests around malformed body, invalid media types etc for the http api Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37192/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37192: More basic call validation tests for http api
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/#review94952 --- Patch looks great! Reviews applied: [37082, 37192] All tests passed. - Mesos ReviewBot On Aug. 11, 2015, 5:04 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/ --- (Updated Aug. 11, 2015, 5:04 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Added more basic call validation tests around malformed body, invalid media types etc for the http api Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37192/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37308: Added AppcImageManifest protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37308/#review94953 --- include/mesos/mesos.proto (line 1402) https://reviews.apache.org/r/37308/#comment149681 Let's remove those fields that are not implemented for now. The JSON-protobuf parsing will fail if those fields are specified. I think that's a simple yet useful behavior (comparing to checking those fields are not set manully). You many want to drop a TODO somewhere saying that the rest of the fields will be added when they are supported. - Jie Yu On Aug. 10, 2015, 6:36 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37308/ --- (Updated Aug. 10, 2015, 6:36 p.m.) Review request for mesos, Ian Downes and Jie Yu. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Derived from /r/34142/. Diffs - include/mesos/mesos.proto 80f56ac2178b24ff19f57c1ace13f567843c7807 Diff: https://reviews.apache.org/r/37308/diff/ Testing --- Tested along with /r/37310/. Thanks, Jiang Yan Xu
Re: Review Request 35668: Report unevictable memory in container statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35668/#review94955 --- Ship it! Ship It! - Jie Yu On June 19, 2015, 9:03 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35668/ --- (Updated June 19, 2015, 9:03 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Bugs: mesos-2798 https://issues.apache.org/jira/browse/mesos-2798 Repository: mesos Description --- Report unevictable memory in container statistics. Diffs - include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19 src/slave/containerizer/isolators/cgroups/mem.cpp 9d65bf5b64ce72c1ca9a7a50e65a357e098af63e Diff: https://reviews.apache.org/r/35668/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 5:03 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Change to use the RecordIO Decoder + Minor cleanup of tests Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/master/master.hpp 10cc100b1cf10aa7a8a9979a2562a9c5a9e55bd6 src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37192: More basic call validation tests for http api
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/ --- (Updated Aug. 11, 2015, 5:04 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Rebased Repository: mesos Description --- Added more basic call validation tests around malformed body, invalid media types etc for the http api Diffs (updated) - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37192/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37307: Changed Image::AppC::id from required to optional.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37307/#review94939 --- Ship it! Ship It! - Jie Yu On Aug. 10, 2015, 6:31 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37307/ --- (Updated Aug. 10, 2015, 6:31 p.m.) Review request for mesos, Ian Downes, Jie Yu, Timothy Chen, and Vinod Kone. Bugs: MESOS-3192 https://issues.apache.org/jira/browse/MESOS-3192 Repository: mesos Description --- See the ticket for details. Diffs - include/mesos/mesos.proto 80f56ac2178b24ff19f57c1ace13f567843c7807 Diff: https://reviews.apache.org/r/37307/diff/ Testing --- N/A. Thanks, Jiang Yan Xu
Re: Review Request 37302: Deleted old style message handling from the scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37302/ --- (Updated Aug. 11, 2015, 5:58 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- updated summary -- @vinodkone. Summary (updated) - Deleted old style message handling from the scheduler library. Repository: mesos Description --- Delete receive handlers ,install call back setups. Helps ease of review in diff for r37303 Diffs - src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830 Diff: https://reviews.apache.org/r/37302/diff/ Testing --- Thanks, Anand Mazumdar
Re: Review Request 37273: [2/3]Add CMake macro VsBuildCommand in libprocess.
On Aug. 10, 2015, 6:42 p.m., Alex Clemmer wrote: 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 77 https://reviews.apache.org/r/37273/diff/2/?file=1036047#file1036047line77 So, why change the value to `TRUE` here? Is there some consequence of this, or is it just clearer to you? (Also, if we want to go this way, it probably makes sense to change the `` we use in the `ExternalProject_Add` calls as well, just for consistency.) haosdent huang wrote: Hmm, because I find ``` set(GLOG_CONFIG_CMD ) ``` the visual studio would still execute default configure step which requires CMakeLists.txt . I still don't know why its behaivour would become this here. Alex Clemmer wrote: Interesting! What does it say on your machine. On my machine (I will have to confirm this in a minute) I remember it saying that there is no configure step for glog. Do you see something different? haosdent huang wrote: LoL, I suck on this strange point in serveral hours and try possible ways to skip it. But all of them are failed. If I direct pass to ExternalProject_Add, it could skip the configuration step. But I set to a variable and pass this variable as configuration step in ExternalProject_Add. It would become try to use default CMake configuration step, which need CMakeLists.txt. I use message to debug the variable and compare the variables with , I am sure the variable is setted to empty string. My CMake version is 3.3; Visual studio version is 2015 community version; OS is win7; Project code is in a independent disk. haosdent huang wrote: I still try to find the reason recently. When I set(GLOG_CONFIG_CMD ), the step become this in Visual Studio. ``` Message Condition='$(Configuration)|$(Platform)'=='Debug|Win32'Performing configure step for 'glog-0.3.3'/Message Command Condition='$(Configuration)|$(Platform)'=='Debug|Win32'setlocal cd Y:\workspace\cpp\mesos\build\3rdparty\libprocess\3rdparty\glog-0.3.3\src\glog-0.3.3-build if %errorlevel% neq 0 goto :cmEnd Y: if %errorlevel% neq 0 goto :cmEnd C:\Program Files (x86)\CMake\bin\cmake.exe -GVisual Studio 14 2015 Y:/workspace/cpp/mesos/build/3rdparty/libprocess/3rdparty/glog-0.3.3/src/glog-0.3.3 if %errorlevel% neq 0 goto :cmEnd C:\Program Files (x86)\CMake\bin\cmake.exe -E touch Y:/workspace/cpp/mesos/build/3rdparty/libprocess/3rdparty/glog-0.3.3/src/glog-0.3.3-stamp/$(Configuration)/glog-0.3.3-configure if %errorlevel% neq 0 goto :cmEnd :cmEnd endlocal amp; call :cmErrorLevel %errorlevel% amp; goto :cmDone :cmErrorLevel exit /b %1 :cmDone if %errorlevel% neq 0 goto :VCEnd/Command ``` - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/#review94776 --- On Aug. 10, 2015, 9:50 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/ --- (Updated Aug. 10, 2015, 9:50 a.m.) Review request for mesos and Alex Clemmer. Repository: mesos Description --- Add CMake macro VsBuildCommand in libprocess. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 76b8c0fe3b4615371e265bab713d62c896b7c3d6 3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION Diff: https://reviews.apache.org/r/37273/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review94969 --- src/slave/containerizer/provisioners/appc.cpp (lines 471 - 481) https://reviews.apache.org/r/34142/#comment149709 Will move this into the bind mount backend. - Jiang Yan Xu On July 7, 2015, 12:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36837: Update gmock to 1.7.0.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36837/#review94974 --- 3rdparty/libprocess/3rdparty/CMakeLists.txt (line 166) https://reviews.apache.org/r/36837/#comment149713 This line breaks the build on my machine OS X 10.10, and I'm not sure how to fix it because CMake's weird string eval semantics make it hard to just pass a normal quoted string in. Does this break on your machien too? - Alex Clemmer On Aug. 7, 2015, 3:24 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36837/ --- (Updated Aug. 7, 2015, 3:24 a.m.) Review request for mesos and Michael Park. Bugs: MESOS-3141 https://issues.apache.org/jira/browse/MESOS-3141 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 711809e808ebd0ed95d62270220e016ba6f41dca 3rdparty/libprocess/3rdparty/gmock-1.6.0.tar.gz d45d9894b0214f5f02a88f6da5c258327110efd8 3rdparty/libprocess/3rdparty/gmock-1.7.0.tar.gz PRE-CREATION 3rdparty/libprocess/3rdparty/versions.am 97727537778511ca5a10be4f3c25cd21d919 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 3c1bb0bfed7e31440dc4be5ee9e3df4ae9152c5c 3rdparty/libprocess/configure.ac 40f344c6847424ea9b68e3d368497bf2763b4c6a Diff: https://reviews.apache.org/r/36837/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 36867: Add labels to FrameworkInfo.
On July 27, 2015, 11:36 p.m., Adam B wrote: Great first patch. Thanks for updating FrameworkInfo on reregistration with the master too! A handful of nits in my first pass. I'll take another look once you've simplified the tests with Kapil's suggestions. Niklas Nielsen wrote: Any updates here? :) Adam B wrote: Looks like @neilc or somebody resolved some of these issues, but I don't see a new patch revision with the changes. Neil? Ping - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36867/#review93242 --- On July 27, 2015, 6:25 p.m., Neil Conway wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36867/ --- (Updated July 27, 2015, 6:25 p.m.) Review request for mesos, Adam B, Connor Doyle, and Niklas Nielsen. Bugs: MESOS-2841 https://issues.apache.org/jira/browse/MESOS-2841 Repository: mesos Description --- This is intended to support frameworks that want to offer capabilities to the rest of the cluster (e.g., storage or some arbitrary third-party service). We want processes running in the cluster to be able to discover these capabilities; however, we don't want to commit to a fixed set of capabilities or how those capabilities should be represented. Hence, this commit represents this information using freeform key-value pairs, similar to the labels mechanism already in use elsewhere. Jira: MESOS-2841 Diffs - include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 2c924addfb4c52d3048ee6ded13ce638145cc93f src/tests/fault_tolerance_tests.cpp 7b977f5e8195d9f42b21f36eb36fb156471caa20 src/tests/master_tests.cpp 05c148ee1660b86428afe4eda718b17052743a8c Diff: https://reviews.apache.org/r/36867/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 37303: Moved scheduler library to http
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/#review94959 --- src/scheduler/scheduler.cpp (lines 328 - 332) https://reviews.apache.org/r/37303/#comment149702 can you use the response decoder here? src/scheduler/scheduler.cpp (line 329) https://reviews.apache.org/r/37303/#comment149689 indent by 4 spaces. src/scheduler/scheduler.cpp (line 330) https://reviews.apache.org/r/37303/#comment149690 indent by 4 spaces. src/scheduler/scheduler.cpp (line 346) https://reviews.apache.org/r/37303/#comment149691 also print the response.get().status? src/scheduler/scheduler.cpp (line 436) https://reviews.apache.org/r/37303/#comment149694 why is this method private but most others are protected? src/scheduler/scheduler.cpp (line 447) https://reviews.apache.org/r/37303/#comment149693 const? src/scheduler/scheduler.cpp (line 475) https://reviews.apache.org/r/37303/#comment149685 as discussed, please make this a paramter to Mesos() constructor. add a TODO for now and follow up in a different patch. - Vinod Kone On Aug. 11, 2015, 12:17 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/ --- (Updated Aug. 11, 2015, 12:17 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Bugs: MESOS-2552 https://issues.apache.org/jira/browse/MESOS-2552 Repository: mesos Description --- This changes moves scheduler library to http. The change to remove auth + install,receive handlers are in other reviews. Diffs - src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830 Diff: https://reviews.apache.org/r/37303/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37328: Remove namespace ambiguity
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37328/#review94970 --- Ship it! src/tests/common/http_tests.cpp (lines 38 - 40) https://reviews.apache.org/r/37328/#comment149710 reorder alphabetically. - Vinod Kone On Aug. 11, 2015, 3:24 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37328/ --- (Updated Aug. 11, 2015, 3:24 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Remove namespace ambiguity. Needed for r37303 Diffs - src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e Diff: https://reviews.apache.org/r/37328/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37304: Add authorization for http based schedulers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37304/#review94968 --- Ship it! I think this can be pulled out of this chain to commit it sooner. i can make the changes and commit it for you, if you like. src/master/master.cpp (line 1828) https://reviews.apache.org/r/37304/#comment149707 indentation. put HttpConnection on the next line? src/master/master.cpp (line 2094) https://reviews.apache.org/r/37304/#comment149708 ditto. indentation. - Vinod Kone On Aug. 11, 2015, 3:24 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37304/ --- (Updated Aug. 11, 2015, 3:24 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- See summary Diffs - src/master/master.hpp 10cc100b1cf10aa7a8a9979a2562a9c5a9e55bd6 src/master/master.cpp 08dd34d9d18f547c6e8d04caf9e39a2b3ffc5f63 Diff: https://reviews.apache.org/r/37304/diff/ Testing --- make check + would add tests in separate patch Thanks, Anand Mazumdar
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/#review94958 --- Ship it! Couple of comments below, but I'll make the adjustments and get this committed for you shortly! Thanks for splitting this out and updating the test! 3rdparty/libprocess/include/process/http.hpp (line 120) https://reviews.apache.org/r/37097/#comment149686 We're still wrapping comments at 70 for now, but that might change soon :) 3rdparty/libprocess/include/process/http.hpp (line 122) https://reviews.apache.org/r/37097/#comment149684 Which RFC? :) 3rdparty/libprocess/include/process/http.hpp (lines 124 - 125) https://reviews.apache.org/r/37097/#comment149683 This bit seems to just be re-iterating the summary above? 3rdparty/libprocess/src/http.cpp (lines 134 - 135) https://reviews.apache.org/r/37097/#comment149692 Reading through the RFC again, it's quite a bit more subtle than this: ``` 4. The identity content-coding is always acceptable, unless specifically refused because the Accept-Encoding field includes identity;q=0, or because the field includes *;q=0 and does not explicitly include the identity content-coding. If the Accept-Encoding field-value is empty, then only the identity encoding is acceptable. If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if identity is one of the available content-codings, then the server SHOULD use the identity content-coding, unless it has additional information that a different content-coding is meaningful to the client. Note: If the request does not include an Accept-Encoding field, and if the identity content-coding is unavailable, then content-codings commonly understood by HTTP/1.0 clients (i.e., gzip and compress) are preferred; some older clients improperly display messages sent with other content-codings. The server might also make this decision based on information about the particular user-agent or client. Note: Most HTTP/1.0 applications do not recognize or obey qvalues associated with content-codings. This means that qvalues will not work and are not permitted with x-gzip or x-compress. ``` So it appears that we're doing the right thing here by returning false and using the identity encoding, but we don't correctly deal with explicitly rejected identity encoding for now.. I'll remove this and the TODO since it's a bit misleading 3rdparty/libprocess/src/http.cpp (lines 145 - 147) https://reviews.apache.org/r/37097/#comment149688 Shouldn't this be up where we're returning false..? 3rdparty/libprocess/src/http.cpp (line 159) https://reviews.apache.org/r/37097/#comment149701 Hm.. it's a bit implicit that tokens.empty is guaranteed to be false here because 'encoding' itself is non-empty due to it coming from tokenize. - Ben Mahler On Aug. 7, 2015, 6:52 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 7, 2015, 6:52 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37335: Fixed scheduler library tests after moving to http
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37335/#review94976 --- src/tests/scheduler_tests.cpp (lines 168 - 170) https://reviews.apache.org/r/37335/#comment149715 IIUC, sending a subscribe call closes the previous reader/stream. How come it receives an error event on the new stream? - Vinod Kone On Aug. 11, 2015, 5:40 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37335/ --- (Updated Aug. 11, 2015, 5:40 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Bugs: MESOS-2552 https://issues.apache.org/jira/browse/MESOS-2552 Repository: mesos Description --- We now let the original scheduler know if some other instance of it failed over. Fixed a test to ignore this error event. Diffs - src/tests/scheduler_tests.cpp 3f01c060045b36b90d027ea3ecfef887ee5f145f Diff: https://reviews.apache.org/r/37335/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 8:11 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Review comments Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/master/master.hpp 6bd05b1f364affeb4f23baa7cdf3a0d00c45a2c6 src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94880 --- Ship it! src/hdfs/hdfs.hpp (lines 69 - 88) https://reviews.apache.org/r/36979/#comment149549 Let's actually capture the error message because when debugging it'll be super nice to see why it failed in the logs instead of nothing (so kill the simplification comment and if you're concerned that someone will just come around and simplify it later leave a comment why we're capturing `out.error()`. Then let's just return `true` if not an error to keep with previous semantics (I don't know the details of `hadoop version` to suggest otherwise, so no need to stray). src/hdfs/hdfs.hpp (line 80) https://reviews.apache.org/r/36979/#comment149550 Not your bug but do you mind s/if(/if (/ please, thanks! src/hdfs/hdfs.hpp (lines 103 - 105) https://reviews.apache.org/r/36979/#comment149551 Instead of this comment I think you could add a comment that explains why we want the output for debugging! src/hdfs/hdfs.hpp (lines 123 - 127) https://reviews.apache.org/r/36979/#comment149552 Wait, how was `|| true` the existing semantics? We are definitely capturing stderr into stdout, but I don't see anything else here unless I'm missing something? src/hdfs/hdfs.hpp (line 159) https://reviews.apache.org/r/36979/#comment149553 This is a really pesky nit, but in the above functions you decided to call the variable capturing the result of `os::shell` to be called `out`, but here and below you decided `output`? Let's pick one and be consistent per this file please. src/tests/containerizer/port_mapping_tests.cpp (line 975) https://reviews.apache.org/r/36979/#comment149554 Minor nit, how about here and below: ASSERT_FALSE(strings::contains(invalid.error(), 256)); - Benjamin Hindman On Aug. 6, 2015, 6:24 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 6, 2015, 6:24 p.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Updating all references to os::shell For more details see MESOS-3142. Diffs - src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7 src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce src/slave/containerizer/isolators/network/port_mapping.cpp 8244c345b84108af7fa18d20e71401d6e1a0aeb0 src/tests/containerizer/isolator_tests.cpp ff6e2b7e190a58a4809d6e71addb15dabe418e17 src/tests/containerizer/port_mapping_tests.cpp 4bee74acba2b1472c80cabbc9d0384bd04c543aa Diff: https://reviews.apache.org/r/36979/diff/ Testing --- make check *Note*: this patch fixes breakages introduce by the refactoring in: https://reviews.apache.org/r/36978 Thanks, Marco Massenzio
Re: Review Request 37002: Introduced ACL protobuf definitions for dynamic reservation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37002/#review95010 --- Ship it! LGTM module comments. FYI - Adam is out for the next several weeks, but I think @Jie can shepherd this? include/mesos/mesos.proto (line 1120) https://reviews.apache.org/r/37002/#comment149765 is this comment here right? or a copypaste fail? :) include/mesos/mesos.proto (lines 1167 - 1168) https://reviews.apache.org/r/37002/#comment149766 for repeated fields is usually good practice to have them ending in a plural (`reserves`?) In this case, maybe `reservations`? `reserved_resources`? - Marco Massenzio On Aug. 5, 2015, 9:57 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37002/ --- (Updated Aug. 5, 2015, 9:57 a.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3062 https://issues.apache.org/jira/browse/MESOS-3062 Repository: mesos Description --- See summary. Diffs - include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba Diff: https://reviews.apache.org/r/37002/diff/ Testing --- Thanks, Michael Park
Re: Review Request 37315: Added basic authentication documentation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/ --- (Updated Aug. 11, 2015, 10:10 p.m.) Review request for mesos. Changes --- Fixed various issues that Vinod raised Bugs: MESOS-1838 https://issues.apache.org/jira/browse/MESOS-1838 Repository: mesos Description --- Added basic authentication documentation Diffs (updated) - docs/authentication.md PRE-CREATION Diff: https://reviews.apache.org/r/37315/diff/ Testing --- Ran git hook to validate formatting. Thanks, Tim Anderegg
Review Request 37374: Only accept v1 protobufs for HTTP API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37374/ --- Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- See above. Diffs - src/master/http.cpp c2bc69fc07112ab0bcfde5dd110358eff811b7bc src/master/master.hpp ef24bf869a0b0daf0e05d60f6b33c0aad1fb58a5 Diff: https://reviews.apache.org/r/37374/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 37374: Only accept v1 protobufs for HTTP API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37374/#review95015 --- Ship it! LGTM - Anand Mazumdar On Aug. 11, 2015, 10:13 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37374/ --- (Updated Aug. 11, 2015, 10:13 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- See above. Diffs - src/master/http.cpp c2bc69fc07112ab0bcfde5dd110358eff811b7bc src/master/master.hpp ef24bf869a0b0daf0e05d60f6b33c0aad1fb58a5 Diff: https://reviews.apache.org/r/37374/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 37315: Added basic authentication documentation
On Aug. 10, 2015, 9:33 p.m., Vinod Kone wrote: Thanks for doing this. Looking pretty good, just some minor comments. Tim Anderegg wrote: Thanks for taking the time to review, Vinod. I'll make those changes tomorrow. OK, submitted a revision. I'm not sure why the Mesos bot build failed, it seems one file was left behind during the make process, but it was not a file that was touched by this commit. If somehow I mucked things up, I'll try and fix it. - Tim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/#review94799 --- On Aug. 11, 2015, 10:10 p.m., Tim Anderegg wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/ --- (Updated Aug. 11, 2015, 10:10 p.m.) Review request for mesos. Bugs: MESOS-1838 https://issues.apache.org/jira/browse/MESOS-1838 Repository: mesos Description --- Added basic authentication documentation Diffs - docs/authentication.md PRE-CREATION Diff: https://reviews.apache.org/r/37315/diff/ Testing --- Ran git hook to validate formatting. Thanks, Tim Anderegg
Re: Review Request 37110: Enabled the Authorizer to handle Reserve/Unreserve ACLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37110/#review95011 --- src/tests/authorization_tests.cpp (line 360) https://reviews.apache.org/r/37110/#comment149772 not sure what the rules are in tests, but shouldn't you use an `Owned` or `unique_ptr` instead of the naked ptr? src/tests/authorization_tests.cpp (lines 376 - 377) https://reviews.apache.org/r/37110/#comment149773 what happens if the request comes from `foo` and `quz`? do I still get to reserve the resource? Should `quz` get it? shouldn't it? Can you please add a few comments in the methods above (and correspondingly in the tests?) When it comes to security / ACLs, always best to have enough info for the guys who will have to solve issues; that's usually never done with the luxury of time, when it comes to security :) src/tests/authorization_tests.cpp (line 397) https://reviews.apache.org/r/37110/#comment149774 the comment here is wrong src/tests/authorization_tests.cpp (lines 407 - 410) https://reviews.apache.org/r/37110/#comment149776 I'll be honest with you: this looks to me upside down :) I'd have: ANY principal can unreserve NONE's resources. In fact, what would happen if one did: ``` acl4-mutable_principals()-set_type(mesos::ACL::Entity::ANY); acl4-mutable_reserver_principals()-set_type(mesos::ACL::Entity::NONE); ``` And what happens if I do: ``` acl4-mutable_principals()-set_type(mesos::ACL::Entity::NONE); acl4-mutable_reserver_principals()-set_type(mesos::ACL::Entity::NONE); ``` src/tests/authorization_tests.cpp (lines 427 - 430) https://reviews.apache.org/r/37110/#comment149777 isn't this identical to the case above? if not, care to add a comment as to what differs? src/tests/authorization_tests.cpp (line 436) https://reviews.apache.org/r/37110/#comment149778 can we add a test for `ops` trying to unreserve multiple principals' resources? foo, bar and quz's (is this even allowed? if not, let's make sure it fails) - Marco Massenzio On Aug. 5, 2015, 9:58 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37110/ --- (Updated Aug. 5, 2015, 9:58 a.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3062 https://issues.apache.org/jira/browse/MESOS-3062 Repository: mesos Description --- See summary. Diffs - src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0 Diff: https://reviews.apache.org/r/37110/diff/ Testing --- Added tests to `src/tests/authorization_tests.cpp` + `make check` Thanks, Michael Park
Re: Review Request 37303: Moved scheduler library to http
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/ --- (Updated Aug. 11, 2015, 10:18 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Updated to using RecordIO reader now. Also setting reader to none on receiving another subscribe event. Bugs: MESOS-2552 https://issues.apache.org/jira/browse/MESOS-2552 Repository: mesos Description --- This changes moves scheduler library to http. The change to remove auth + install,receive handlers are in other reviews. Diffs (updated) - src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 Diff: https://reviews.apache.org/r/37303/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37315: Added basic authentication documentation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/#review95018 --- Ship it! Thank you. Ignore the bot failure. It's not related to your patch. - Vinod Kone On Aug. 11, 2015, 10:10 p.m., Tim Anderegg wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/ --- (Updated Aug. 11, 2015, 10:10 p.m.) Review request for mesos. Bugs: MESOS-1838 https://issues.apache.org/jira/browse/MESOS-1838 Repository: mesos Description --- Added basic authentication documentation Diffs - docs/authentication.md PRE-CREATION Diff: https://reviews.apache.org/r/37315/diff/ Testing --- Ran git hook to validate formatting. Thanks, Tim Anderegg
Re: Review Request 37315: Added basic authentication documentation
On Aug. 11, 2015, 10:19 p.m., Vinod Kone wrote: Thank you. Ignore the bot failure. It's not related to your patch. Cool, I thought as much, thanks again for the feedback. - Tim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/#review95018 --- On Aug. 11, 2015, 10:10 p.m., Tim Anderegg wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37315/ --- (Updated Aug. 11, 2015, 10:10 p.m.) Review request for mesos. Bugs: MESOS-1838 https://issues.apache.org/jira/browse/MESOS-1838 Repository: mesos Description --- Added basic authentication documentation Diffs - docs/authentication.md PRE-CREATION Diff: https://reviews.apache.org/r/37315/diff/ Testing --- Ran git hook to validate formatting. Thanks, Tim Anderegg
Re: Review Request 37125: Added 'Master::authorize' for Reserve/Unreserve.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37125/#review95020 --- Ship it! LGTM modulo my (blocking) concern about allowing a malicious user to crash master on a malformed request. src/master/master.cpp (line 2395) https://reviews.apache.org/r/37125/#comment149783 are you sure about this? this will cause master to abort on a malformed request - wouldn't it be best to take the 'safe route' and just deny authorization? security best practice is usually just say no when you don't understand :) (and don't provide even any feedback other than Not Authorized to a potential attacker). - Marco Massenzio On Aug. 5, 2015, 10 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37125/ --- (Updated Aug. 5, 2015, 10 a.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3062 https://issues.apache.org/jira/browse/MESOS-3062 Repository: mesos Description --- The `Master::authorize` function is overloaded for `Reserve` and `Unreserve`. This will be extended for `Create` and `Destroy` in the future. These are used for authorization of frameworks as well as master endpoints. Diffs - src/master/master.hpp e44174976aa64176916827bec4c911333c9a91db src/master/master.cpp 5aa0a5410804fe16abd50b6953f1ffe46a019ecf Diff: https://reviews.apache.org/r/37125/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 37374: Only accept v1 protobufs for HTTP API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37374/#review95022 --- Ship it! Ship It! - Vinod Kone On Aug. 11, 2015, 10:13 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37374/ --- (Updated Aug. 11, 2015, 10:13 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- See above. Diffs - src/master/http.cpp c2bc69fc07112ab0bcfde5dd110358eff811b7bc src/master/master.hpp ef24bf869a0b0daf0e05d60f6b33c0aad1fb58a5 Diff: https://reviews.apache.org/r/37374/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 37309: Add app::paths which handles Appc related path manipulation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37309/#review95023 --- src/slave/containerizer/provisioners/appc/paths.hpp (line 55) https://reviews.apache.org/r/37309/#comment149785 s/id/imageId/ src/slave/containerizer/provisioners/appc/paths.hpp (line 60) https://reviews.apache.org/r/37309/#comment149786 s/id/imageId/ src/slave/containerizer/provisioners/appc/paths.hpp (line 68) https://reviews.apache.org/r/37309/#comment149787 s/id/imageId/ - Jie Yu On Aug. 10, 2015, 6:38 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37309/ --- (Updated Aug. 10, 2015, 6:38 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. Repository: mesos Description --- - Akin to slave::paths. Note that more paths manipulation logic are going to be added to handle things such as the staging directory and the local image repository layout. Diffs - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/paths.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/paths.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37309/diff/ Testing --- Tested along with /r/37310/. Thanks, Jiang Yan Xu
Re: Review Request 37199: Added store interface and moved store implementation to LocalStore subclass.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37199/ --- (Updated Aug. 11, 2015, 11:24 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2849 https://issues.apache.org/jira/browse/MESOS-2849 Repository: mesos Description --- Added store interface and moved store implementation to LocalStore subclass. Diffs (updated) - src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37199/diff/ Testing --- make check Thanks, Lily Chen
Review Request 37377: Disallow HTTP schedulers when authentication is required.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/ --- Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- Since we don't have authentication implemented yet, this disallows http schedulers when authentication is required. Diffs - src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37377/diff/ Testing --- Updated the tests. Thanks, Ben Mahler
Re: Review Request 37126: Added authorization for dynamic reservation master endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37126/#review95021 --- src/master/http.cpp (line 593) https://reviews.apache.org/r/37126/#comment149784 feel free to make fun of my ignorance... but why not a real lambda function here? (and below too) src/tests/reservation_endpoints_tests.cpp (line 980) https://reviews.apache.org/r/37126/#comment149788 nit: s/set-up/setup src/tests/reservation_endpoints_tests.cpp (line 1026) https://reviews.apache.org/r/37126/#comment149791 nit: I believe we prefer the use of `using` for std::string? (I'm sure there's already one about 1,000 lines above here...) - Marco Massenzio On Aug. 5, 2015, 10:46 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37126/ --- (Updated Aug. 5, 2015, 10:46 a.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3062 https://issues.apache.org/jira/browse/MESOS-3062 Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3 src/master/master.hpp e44174976aa64176916827bec4c911333c9a91db src/tests/reservation_endpoints_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37126/diff/ Testing --- Added tests to `src/tests/reservation_endpoints_tests.cpp` + `make check`. Thanks, Michael Park
Re: Review Request 37247: Added Docker image reference store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37247/#review95034 --- Bad patch! Reviews applied: [37196, 37197, 37198, 37199, 37200, 37245, 37246, 37247] Failed command: ./support/apply-review.sh -n -r 37247 Error: 2015-08-12 00:28:40 URL:https://reviews.apache.org/r/37247/diff/raw/ [15655/15655] - 37247.patch [1] error: patch failed: src/Makefile.am:422 error: src/Makefile.am: patch does not apply Failed to apply patch - Mesos ReviewBot On Aug. 11, 2015, 11:34 p.m., Lily Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37247/ --- (Updated Aug. 11, 2015, 11:34 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-3021 https://issues.apache.org/jira/browse/MESOS-3021 Repository: mesos Description --- Added Docker image reference store. Diffs - src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 src/messages/docker_provisioner.hpp PRE-CREATION src/messages/docker_provisioner.proto PRE-CREATION src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37247/diff/ Testing --- make check Tests will be added in a later review. Thanks, Lily Chen
Re: Review Request 37266: Style checker checking for { on newline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37266/
---
(Updated Aug. 12, 2015, 1:16 a.m.)
Review request for mesos.
Bugs: MESOS-2578
https://issues.apache.org/jira/browse/MESOS-2578
Repository: mesos
Description (updated)
---
As requested in the issue MESOS-2578 the style checker now
verifies { on newline for class and methods declarations.
This commit contains the files changed in the mesos project
Diffs
-
src/authentication/cram_md5/authenticator.cpp
6a84e9184df837cd90ac7485b88ae7f47e12537b
src/docker/executor.hpp fa13b6e9905051eef27d3a51b75a5c86fdad0dd7
src/linux/cgroups.cpp e062fcbd56315f11882fe0ccb615c490dd719934
src/logging/logging.cpp fb798670d9ac79c75ad39905614fbfe1ea25fba6
src/python/native/src/mesos/native/module.hpp
31da47b474d017e910d90e41ad15f2163b07dc89
src/slave/containerizer/isolators/network/port_mapping.cpp
88c0cbc61f3f97b084cc3b3fae8999b07d4aa1c7
src/tests/containerizer/docker_containerizer_tests.cpp
80ed60e2b0fa39e8302867a7cb6a7388c25f9a40
src/tests/containerizer/rootfs.hpp 55dd4964cfb1ca0e5f7b7616ccc6d5ad2be135d7
src/tests/environment.hpp 9cf14bcc8f7d386f6aa26b686d3f953c969aaf63
support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b
support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7
Diff: https://reviews.apache.org/r/37266/diff/
Testing
---
Thanks,
José Guilherme Vanz
Re: Review Request 37268: Style checker checking for { on newline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37268/
---
(Updated Aug. 12, 2015, 1:15 a.m.)
Review request for mesos.
Bugs: MESOS-2578
https://issues.apache.org/jira/browse/MESOS-2578
Repository: mesos
Description (updated)
---
As requested in the issue MESOS-2578 the style checker now
verifies { on newline for class and methods declarations.
This commit contains the files changed in the stout project
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp
30baa65837621a277cf9d1042a751bfe18004b05
Diff: https://reviews.apache.org/r/37268/diff/
Testing
---
Thanks,
José Guilherme Vanz
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review95052 --- Ship it! Ship It! - Vinod Kone On Aug. 11, 2015, 10:57 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 10:57 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Review Request 37382: Introduced provisioner Backend interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37382/ --- Review request for mesos, Lily Chen, Ian Downes, Jie Yu, and Timothy Chen. Bugs: MESOS-2968 https://issues.apache.org/jira/browse/MESOS-2968 Repository: mesos Description --- The Backend interface is made generic so it can be used by different provisioners. See [MESOS-2968](https://issues.apache.org/jira/browse/MESOS-2968?focusedCommentId=14652859page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14652859) for details. Diffs - src/Makefile.am 07502f0f5523b972888ceab9cf3af309c8441d7f src/slave/containerizer/provisioners/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/backend.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37382/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37200/ --- (Updated Aug. 11, 2015, 11:26 p.m.) Review request for mesos and Timothy Chen. Changes --- Changed call to store-put to use sandbox to be consistent with store interface. Bugs: MESOS-2849 https://issues.apache.org/jira/browse/MESOS-2849 Repository: mesos Description --- Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers. Diffs (updated) - src/slave/containerizer/provisioners/docker.hpp PRE-CREATION src/slave/containerizer/provisioners/docker.cpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e Diff: https://reviews.apache.org/r/37200/diff/ Testing --- make check Thanks, Lily Chen
Re: Review Request 37133: Add a frameworks parameter to the hierarchical allocator benchmark.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37133/#review95027 --- Ship it! Sorry this took so long, I'll get this committed for you now. src/tests/hierarchical_allocator_tests.cpp (line 1119) https://reviews.apache.org/r/37133/#comment149806 This comment seems a bit misleading, since each slave only has allocation for 1 framework. I'll re-phrase it. - Ben Mahler On Aug. 5, 2015, 5:34 p.m., James Peach wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37133/ --- (Updated Aug. 5, 2015, 5:34 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3209 https://issues.apache.org/jira/browse/MESOS-3209 Repository: mesos Description --- Parameterize the hierarchical allocator benchmark by the framework count as well as the slave count. This can be used to explore allocation behavior as the number of frameworks increases. Diffs - src/tests/hierarchical_allocator_tests.cpp c92d47aa0a06088f1f8fe749a629c27bfadd3c6d Diff: https://reviews.apache.org/r/37133/diff/ Testing --- make check make bench Thanks, James Peach
Re: Review Request 37303: Moved scheduler library to http
On Aug. 11, 2015, 6:55 p.m., Vinod Kone wrote: src/scheduler/scheduler.cpp, line 329 https://reviews.apache.org/r/37303/diff/2/?file=1036937#file1036937line329 indent by 4 spaces. Not used now. Fixed the other one. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/#review94959 --- On Aug. 11, 2015, 10:18 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/ --- (Updated Aug. 11, 2015, 10:18 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Bugs: MESOS-2552 https://issues.apache.org/jira/browse/MESOS-2552 Repository: mesos Description --- This changes moves scheduler library to http. The change to remove auth + install,receive handlers are in other reviews. Diffs - src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 Diff: https://reviews.apache.org/r/37303/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37247: Added Docker image reference store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37247/ --- (Updated Aug. 11, 2015, 11:34 p.m.) Review request for mesos and Timothy Chen. Changes --- Moved DockerProvisionerImages to a .proto file not accessible through the public API and added some logs. Bugs: MESOS-3021 https://issues.apache.org/jira/browse/MESOS-3021 Repository: mesos Description --- Added Docker image reference store. Diffs (updated) - src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 src/messages/docker_provisioner.hpp PRE-CREATION src/messages/docker_provisioner.proto PRE-CREATION src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37247/diff/ Testing --- make check Tests will be added in a later review. Thanks, Lily Chen
Re: Review Request 37377: Disallow HTTP schedulers when authentication is required.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/#review95029 --- Ship it! LGTM ! - Anand Mazumdar On Aug. 11, 2015, 11:52 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/ --- (Updated Aug. 11, 2015, 11:52 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- Since we don't have authentication implemented yet, this disallows http schedulers when authentication is required. Diffs - src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37377/diff/ Testing --- Updated the tests. Thanks, Ben Mahler
Re: Review Request 37267: Style checker checking for { on newline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37267/
---
(Updated Aug. 12, 2015, 1:16 a.m.)
Review request for mesos.
Bugs: MESOS-2578
https://issues.apache.org/jira/browse/MESOS-2578
Repository: mesos
Description (updated)
---
As requested in the issue MESOS-2578 the style checker now
verifies { on newline for class and methods declarations.
This commit contains the files changed in the libprocess project
Diffs
-
3rdparty/libprocess/include/process/filter.hpp
db0dfc7ae89245b748337c53e524f3cb352ed301
3rdparty/libprocess/include/process/future.hpp
db92767619ec7b6ab1a0803c240725a2633d2489
3rdparty/libprocess/include/process/metrics/metric.hpp
c5e61df09b06ff13695646eb97c69235a4fe8d56
3rdparty/libprocess/include/process/process.hpp
bf8e2bf46fad2eae1c9f1b788b2b71305664e508
Diff: https://reviews.apache.org/r/37267/diff/
Testing
---
Thanks,
José Guilherme Vanz
Re: Review Request 37378: Updated http api tests to use V1 Protobufs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37378/#review95053 --- Ship it! Ship It! - Vinod Kone On Aug. 12, 2015, 12:14 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37378/ --- (Updated Aug. 12, 2015, 12:14 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Updated to use v1 protobufs for http api tests. Added a default conversion function for FrameworkInfo in devolve() Diffs - src/internal/devolve.hpp b9a854a5e2454a4c3f2b85032b928b2cbc0b000c src/internal/devolve.cpp be74a26e9a194a8d969c667fdc084964939dfdb7 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37378/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37311/#review95005
---
src/slave/containerizer/provisioners/appc/store.hpp (line 39)
https://reviews.apache.org/r/37311/#comment149753
Put '{' in a new line. Please fix all occurances in this file.
src/slave/containerizer/provisioners/appc/store.hpp (lines 39 - 55)
https://reviews.apache.org/r/37311/#comment149762
Can you Move this struct to be a nested struct in Store? (i.e.,
Store::Image)
src/slave/containerizer/provisioners/appc/store.hpp (line 46)
https://reviews.apache.org/r/37311/#comment149755
Can those be 'const'?
src/slave/containerizer/provisioners/appc/store.hpp (line 60)
https://reviews.apache.org/r/37311/#comment149756
One extra line please.
src/slave/containerizer/provisioners/appc/store.hpp (line 61)
https://reviews.apache.org/r/37311/#comment149760
Could you please add some comments about this class? It's not obvious what
this is for.
src/slave/containerizer/provisioners/appc/store.cpp (line 50)
https://reviews.apache.org/r/37311/#comment149769
No need to have a static 'create' function here since this class is in the
cpp file. You can just make the constructor public.
src/slave/containerizer/provisioners/appc/store.cpp (line 60)
https://reviews.apache.org/r/37311/#comment149770
Maybe s/store/root/ and add some comments about that?
src/slave/containerizer/provisioners/appc/store.cpp (line 108)
https://reviews.apache.org/r/37311/#comment149779
Please add a comment explaining that mkdir will return Nothing if images
dir already exist.
Do you want to do a os::exists check first to check if flags.appc_store_dir
exists?
src/slave/containerizer/provisioners/appc/store.cpp (lines 129 - 134)
https://reviews.apache.org/r/37311/#comment149793
No need to call 'realpath' here every time. We just need to make sure
'root' is a canonicalized realpath.
src/slave/containerizer/provisioners/appc/store.cpp (line 141)
https://reviews.apache.org/r/37311/#comment149794
s/id/imageId/
src/slave/containerizer/provisioners/appc/store.cpp (lines 141 - 144)
https://reviews.apache.org/r/37311/#comment149795
Hum, let's merge this function into 'recover' for now since it's the only
caller.
IN that way, you can get rid of this code because imageId is available
already.
src/slave/containerizer/provisioners/appc/store.cpp (line 171)
https://reviews.apache.org/r/37311/#comment149781
`s/images_/result/`
src/slave/containerizer/provisioners/appc/store.cpp (line 183)
https://reviews.apache.org/r/37311/#comment149789
s/entries/imageIds/
src/slave/containerizer/provisioners/appc/store.cpp (line 185)
https://reviews.apache.org/r/37311/#comment149782
storage entry is a little confusing to me. How about:
```
return Failure(
Failed to list all images under ' +
paths::getImagesDir(root) + ': +
entries.error());
```
src/slave/containerizer/provisioners/appc/store.cpp (line 188)
https://reviews.apache.org/r/37311/#comment149790
s/entry/imageId
src/tests/containerizer/appc_provisioner_tests.cpp (line 156)
https://reviews.apache.org/r/37311/#comment149799
s/id/imageId
src/tests/containerizer/appc_provisioner_tests.cpp (line 160)
https://reviews.apache.org/r/37311/#comment149798
s/image/imagePath/
src/tests/containerizer/appc_provisioner_tests.cpp (line 169)
https://reviews.apache.org/r/37311/#comment149797
`s/images_/_images`
- Jie Yu
On Aug. 10, 2015, 7:19 p.m., Jiang Yan Xu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37311/
---
(Updated Aug. 10, 2015, 7:19 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen.
Bugs: MESOS-3194
https://issues.apache.org/jira/browse/MESOS-3194
Repository: mesos
Description
---
Implemented a 'read-only' Appc image store.
Diffs
-
src/Makefile.am 942003149b071a322933e2c085d9122903f69713
src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION
src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46
src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e
src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/37311/diff/
Testing
---
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 37336: Added `wait()` method to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review95042 --- Patch looks great! Reviews applied: [37336] All tests passed. - Mesos ReviewBot On Aug. 11, 2015, 11:36 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Aug. 11, 2015, 11:36 p.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added a `wait(Duration timeout)` method that returns a `CommandResult` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. The `wait()` method will wait for both the process to terminate, and stdout/stderr to be available to read from; it also unpacks them into ready-to-use `string`s. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 35702: Added /reserve HTTP endpoint to the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35702/ --- (Updated Aug. 12, 2015, 2:44 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris Van Remoortere, and Vinod Kone. Changes --- Discarded [r36987](https://reviews.apache.org/r/36987/) and rebased accordingly. Bugs: MESOS-2600 https://issues.apache.org/jira/browse/MESOS-2600 Repository: mesos Description --- This involved a lot more challenges than I anticipated, I've captured the various approaches and limitations and deal-breakers of those approaches here: [Master Endpoint Implementation Challenges](https://docs.google.com/document/d/1cwVz4aKiCYP9Y4MOwHYZkyaiuEv7fArCye-vPvB2lAI/edit#) Key points: * This is a stop-gap solution until we shift the offer creation/management logic from the master to the allocator. * `updateAvailable` and `updateSlave` are kept separate because (1) `updateAvailable` is allowed to fail whereas `updateSlave` must not. (2) `updateAvailable` returns a `Future` whereas `updateSlave` does not. (3) `updateAvailable` never leaves the allocator in an over-allocated state and must not, whereas `updateSlave` does, and can. * The algorithm: * Initially, the master pessimistically assume that what seems like available resources will be gone. This is due to the race between the allocator scheduling an `allocate` call to itself vs master's `allocator-updateAvailable` invocation. As such, we first try to satisfy the request only with the offered resources. * We greedily rescind one offer at a time until we've rescinded sufficiently many offers. IMPORTANT: We perform `recoverResources(..., Filters())` which has a default `refuse_sec` of 5 seconds, rather than `recoverResources(..., None())` so that we can virtually always win the race against `allocate`. In the rare case that we do lose, no disaster occurs. We simply fail to satisfy the request. * If we still don't have enough resources after resciding all offers, be semi-optimistic and forward the request to the allocator since there may be available resources to satisfy the request. * If the allocator returns a failure, report the error to the user with `Conflict`. This approach is clearly not ideal, since we would prefer to rescind as little offers as possible. Diffs (updated) - src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37 src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96 src/master/master.cpp 398203d9367f85340166e66ecc34b9a33dd81048 src/master/validation.hpp 43b8d84556e7f0a891dddf6185bbce7ca50b360a src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 Diff: https://reviews.apache.org/r/35702/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 37198: Add Docker image provisioner and copy backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37198/ --- (Updated Aug. 11, 2015, 11:23 p.m.) Review request for mesos and Timothy Chen. Changes --- Rebased on master to be consistent with provisioner interface. Bugs: MESOS-2850 https://issues.apache.org/jira/browse/MESOS-2850 Repository: mesos Description --- Add Docker image provisioner and copy backend. Diffs (updated) - src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 src/slave/containerizer/provisioner.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838 src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e src/slave/containerizer/provisioners/docker.hpp PRE-CREATION src/slave/containerizer/provisioners/docker.cpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e Diff: https://reviews.apache.org/r/37198/diff/ Testing --- make check Thanks, Lily Chen
Re: Review Request 37197: Docker image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37197/ --- (Updated Aug. 11, 2015, 11:21 p.m.) Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen. Bugs: MESOS-2849 https://issues.apache.org/jira/browse/MESOS-2849 Repository: mesos Description --- Stored images currently kept indefinitely. Diffs (updated) - src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e Diff: https://reviews.apache.org/r/37197/diff/ Testing --- make check Thanks, Lily Chen
Re: Review Request 37246: Refactor store to use updated DockerImage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37246/ --- (Updated Aug. 11, 2015, 11:32 p.m.) Review request for mesos and Timothy Chen. Changes --- Converted store interface comments to use javadoc style. Bugs: MESOS-2849 https://issues.apache.org/jira/browse/MESOS-2849 Repository: mesos Description --- Refactor local store to use updated DockerImage. Diffs (updated) - src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37246/diff/ Testing --- make check Thanks, Lily Chen
Re: Review Request 37303: Moved scheduler library to http
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review95036
---
Thanks Anand, mostly thinking we can clean up the read logic if we have a
struct to capture the reader / decoder.
src/common/http.hpp (lines 51 - 53)
https://reviews.apache.org/r/37303/#comment149809
Can't we just have support for stringifying these?
e.g. stringify(ContentType::PROTOBUF) == APPLICATION_PROTOBUF
Or is there something I'm missing?
src/common/http.hpp (lines 80 - 81)
https://reviews.apache.org/r/37303/#comment149808
value.get() need to be called only if value.isSome()
src/scheduler/scheduler.cpp (line 111)
https://reviews.apache.org/r/37303/#comment149810
This is just for testing right? Might be nice to expose as a separate
constructor with a note that it's for testing.
src/scheduler/scheduler.cpp (line 117)
https://reviews.apache.org/r/37303/#comment149812
Hm.. can we avoid the 'headers' variable? Seems clearer to keep it local to
an individual request for now.
src/scheduler/scheduler.cpp (lines 219 - 221)
https://reviews.apache.org/r/37303/#comment149817
This is required, so how about just saying:
```
// Each subscription requires a new connection.
disconnect();
```
src/scheduler/scheduler.cpp (lines 222 - 223)
https://reviews.apache.org/r/37303/#comment149816
Can we call this `disconnect`? Any reason to not clear the reader within
the function rather than in the caller?
src/scheduler/scheduler.cpp (line 323)
https://reviews.apache.org/r/37303/#comment149824
Need to deal with isFailed case before you call .get() as well.
src/scheduler/scheduler.cpp (line 325)
https://reviews.apache.org/r/37303/#comment149820
Looks like you don't need this, how about:
if (call.type() == Call::SUBSCRIBE
response.get().status == process::http::statuses[200]) {
...
} else if (response.get().status == process::http::statuses[202]) {
...
} else {
error!
}
src/scheduler/scheduler.cpp (line 328)
https://reviews.apache.org/r/37303/#comment149818
Can you flip this comparison?
src/scheduler/scheduler.cpp (line 329)
https://reviews.apache.org/r/37303/#comment149819
CHECK_EQ?
src/scheduler/scheduler.cpp (lines 332 - 338)
https://reviews.apache.org/r/37303/#comment149822
Seems like having a higher level struct, like we did with HttpConnection in
the master, will help us here as well?
That can provide a 'read' function that returns a FutureEvent. When we
process the read, we can see if the connection is still the same in order to
decide whether to continue reading.
src/scheduler/scheduler.cpp (lines 353 - 355)
https://reviews.apache.org/r/37303/#comment149821
How about:
```
Received a '500 Internal Error' for SUBSCRIBE call: Body of request
```
Remember that we don't use periods in logging messages
src/scheduler/scheduler.cpp (line 383)
https://reviews.apache.org/r/37303/#comment149826
Failed to decode the stream of events: ...
src/scheduler/scheduler.cpp (lines 390 - 397)
https://reviews.apache.org/r/37303/#comment149825
Why does master disconnection send an Error event? This can occur if the
master crashes, fails over, etc. So the scheduler should not see an error for
this?
src/scheduler/scheduler.cpp (lines 400 - 401)
https://reviews.apache.org/r/37303/#comment149827
s/chunk/event/
How about:
```
Failed to de-serialize event sent by master: ...
```
src/scheduler/scheduler.cpp (line 435)
https://reviews.apache.org/r/37303/#comment149813
Can we call this 'disconnect'?
src/scheduler/scheduler.cpp (line 437)
https://reviews.apache.org/r/37303/#comment149814
space here :)
src/scheduler/scheduler.cpp (line 439)
https://reviews.apache.org/r/37303/#comment149815
Can we avoid saying reader in these messages? Let's just say that the
connection was already closed, since the users of this library don't care about
the implementation detail of there being a Reader.
- Ben Mahler
On Aug. 11, 2015, 10:18 p.m., Anand Mazumdar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/
---
(Updated Aug. 11, 2015, 10:18 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-2552
https://issues.apache.org/jira/browse/MESOS-2552
Repository: mesos
Description
---
This changes moves scheduler library to http. The change to remove auth +
install,receive handlers are in other reviews.
Diffs
-
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/common/http.cpp
Re: Review Request 37245: Refactor Docker Image to exclude path and manifest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37245/ --- (Updated Aug. 11, 2015, 11:29 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2850 https://issues.apache.org/jira/browse/MESOS-2850 Repository: mesos Description --- Refactor Docker Image to exclude path and manifest. Diffs (updated) - src/slave/containerizer/provisioners/docker.hpp PRE-CREATION src/slave/containerizer/provisioners/docker.cpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37245/diff/ Testing --- make check Thanks, Lily Chen
Re: Review Request 37192: More basic call validation tests for http api
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/#review95031 --- Patch looks great! Reviews applied: [37082, 37192] All tests passed. - Mesos ReviewBot On Aug. 11, 2015, 10:58 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37192/ --- (Updated Aug. 11, 2015, 10:58 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Added more basic call validation tests around malformed body, invalid media types etc for the http api Diffs - src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37192/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37336: Added `wait()` method to process::Subprocess
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/ --- (Updated Aug. 11, 2015, 11:36 p.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-3035 The original API for `process::Subprocess` still left a lot of legwork to do for the caller; we have now added a `wait(Duration timeout)` method that returns a `CommandResult` (also introduced with this patch) which contains useful information about the command invocation; the exit code; stdout and, optionally, stderr too. The `wait()` method will wait for both the process to terminate, and stdout/stderr to be available to read from; it also unpacks them into ready-to-use `string`s. Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d Diff: https://reviews.apache.org/r/37336/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 37378: Updated http api tests to use V1 Protobufs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37378/ --- Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Updated to use v1 protobufs for http api tests. Added a default conversion function for FrameworkInfo in devolve() Diffs - src/internal/devolve.hpp b9a854a5e2454a4c3f2b85032b928b2cbc0b000c src/internal/devolve.cpp be74a26e9a194a8d969c667fdc084964939dfdb7 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37378/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37377: Disallow HTTP schedulers when authentication is required.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/#review95047 --- Patch looks great! Reviews applied: [37377] All tests passed. - Mesos ReviewBot On Aug. 11, 2015, 11:52 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/ --- (Updated Aug. 11, 2015, 11:52 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- Since we don't have authentication implemented yet, this disallows http schedulers when authentication is required. Diffs - src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37377/diff/ Testing --- Updated the tests. Thanks, Ben Mahler
Re: Review Request 37303: Moved scheduler library to http
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review95059
---
Sorry for not elaborating on all of these, I added some more explanations here.
Main thing is cleaning up the read loop and figuring out the callback semantics
(do we need to revisit 'connected' / 'disconnected'?).
src/common/http.hpp (lines 56 - 57)
https://reviews.apache.org/r/37303/#comment149859
You can `return stream ...;` in a single statement.
src/common/http.hpp (lines 97 - 99)
https://reviews.apache.org/r/37303/#comment149860
Feel free to just return directly, rather than storing in a local variable.
src/scheduler/scheduler.cpp (line 117)
https://reviews.apache.org/r/37303/#comment149861
Sorry, let me elaborate further. I mentioned not having this because
headers requires non-local reasoning when reading the request sending code:
```
// Send a streaming request for Subscribe call.
response = process::http::streaming::post(
master.get(),
api/v1/scheduler,
headers, // non-local
body,
stringify(contentType));
vs.
// Send a streaming request for Subscribe call.
response = process::http::streaming::post(
master.get(),
api/v1/scheduler,
{{Accept, stringify(contentType)}}, // local
body,
stringify(contentType));
```
We've generally found local reasoning makes code easier to read (a.k.a.
readable). Also, keep in mind that in the future you'll be able to send a
Request directly, so it would become:
```
Request request;
...
request.headers[Accept] = stringify(contentType);
response = process::http::streaming::request(request);
```
The other concern is that headers is not necessarily going to remain
constant like this in the future (e.g. if we add the notion of a session
header). Make sense?
src/scheduler/scheduler.cpp (line 323)
https://reviews.apache.org/r/37303/#comment149863
remove the space here
src/scheduler/scheduler.cpp (line 329)
https://reviews.apache.org/r/37303/#comment149862
The reason I ask to flip this is we've generally not used yoda style
comparisons, e.g.
```
size() 1
rather than
1 size()
```
Can you do a sweep in the code you've introduced here?
src/scheduler/scheduler.cpp (line 330)
https://reviews.apache.org/r/37303/#comment149864
Flip the comparison here as well. Be sure to do a sweep.
src/scheduler/scheduler.cpp (lines 354 - 362)
https://reviews.apache.org/r/37303/#comment149868
I suggested the struct because passing around the recodio reader
independently of the pipe reader seems to be not that intuitive, was hoping to
see them stored together inside an OptionConnection rather than having the
OptionPipe::Reader and recordio::Reader stored separately. Make more sense?
Also, can we use recordio::Reader to be more explicit about this type?
src/scheduler/scheduler.cpp (lines 387 - 394)
https://reviews.apache.org/r/37303/#comment149865
Ok, let's chat about this what to do here, might need to revisit the
callbacks here now that we have http. But this shouldn't be an error since it's
just disconnection. If we call disconnected here though, seems that we need to
immediately call connected if there is still a master available, otherwise we
might hang around when we should be retrying.
src/scheduler/scheduler.cpp (lines 388 - 389)
https://reviews.apache.org/r/37303/#comment149866
There is no end of file chunk.. :)
- Ben Mahler
On Aug. 12, 2015, 4:12 a.m., Anand Mazumdar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/
---
(Updated Aug. 12, 2015, 4:12 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-2552
https://issues.apache.org/jira/browse/MESOS-2552
Repository: mesos
Description
---
This changes moves scheduler library to http. The change to remove auth +
install,receive handlers are in other reviews.
Diffs
-
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316
Diff: https://reviews.apache.org/r/37303/diff/
Testing
---
make check
Thanks,
Anand Mazumdar
Re: Review Request 37268: Style checker checking for { on newline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37268/
---
(Updated Aug. 12, 2015, 1:10 a.m.)
Review request for mesos.
Bugs: MESOS-2578
https://issues.apache.org/jira/browse/MESOS-2578
Repository: mesos
Description (updated)
---
As requested in the issue MESOS-2578 the style checker now
verifies { on newline for class and methods declarations.
This commit contains the files changed in the stout project
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp
30baa65837621a277cf9d1042a751bfe18004b05
Diff: https://reviews.apache.org/r/37268/diff/
Testing
---
Thanks,
José Guilherme Vanz
Re: Review Request 37267: Style checker checking for { on newline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37267/
---
(Updated Aug. 12, 2015, 1:10 a.m.)
Review request for mesos.
Bugs: MESOS-2578
https://issues.apache.org/jira/browse/MESOS-2578
Repository: mesos
Description (updated)
---
As requested in the issue MESOS-2578 the style checker now
verifies { on newline for class and methods declarations.
This commit contains the files changed in the libprocess project
Diffs
-
3rdparty/libprocess/include/process/filter.hpp
db0dfc7ae89245b748337c53e524f3cb352ed301
3rdparty/libprocess/include/process/future.hpp
db92767619ec7b6ab1a0803c240725a2633d2489
3rdparty/libprocess/include/process/metrics/metric.hpp
c5e61df09b06ff13695646eb97c69235a4fe8d56
3rdparty/libprocess/include/process/process.hpp
bf8e2bf46fad2eae1c9f1b788b2b71305664e508
Diff: https://reviews.apache.org/r/37267/diff/
Testing
---
Thanks,
José Guilherme Vanz
Re: Review Request 37266: Style checker checking for { on newline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37266/
---
(Updated Aug. 12, 2015, 1:10 a.m.)
Review request for mesos.
Bugs: MESOS-2578
https://issues.apache.org/jira/browse/MESOS-2578
Repository: mesos
Description (updated)
---
As requested in the issue MESOS-2578 the style checker now
verifies { on newline for class and methods declarations.
This commit contains the files changed in the mesos project
Diffs
-
src/authentication/cram_md5/authenticator.cpp
6a84e9184df837cd90ac7485b88ae7f47e12537b
src/docker/executor.hpp fa13b6e9905051eef27d3a51b75a5c86fdad0dd7
src/linux/cgroups.cpp e062fcbd56315f11882fe0ccb615c490dd719934
src/logging/logging.cpp fb798670d9ac79c75ad39905614fbfe1ea25fba6
src/python/native/src/mesos/native/module.hpp
31da47b474d017e910d90e41ad15f2163b07dc89
src/slave/containerizer/isolators/network/port_mapping.cpp
88c0cbc61f3f97b084cc3b3fae8999b07d4aa1c7
src/tests/containerizer/docker_containerizer_tests.cpp
80ed60e2b0fa39e8302867a7cb6a7388c25f9a40
src/tests/containerizer/rootfs.hpp 55dd4964cfb1ca0e5f7b7616ccc6d5ad2be135d7
src/tests/environment.hpp 9cf14bcc8f7d386f6aa26b686d3f953c969aaf63
support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b
support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7
Diff: https://reviews.apache.org/r/37266/diff/
Testing
---
Thanks,
José Guilherme Vanz
Re: Review Request 37236: Added the linux filesystem isolator.
On Aug. 11, 2015, 9:06 p.m., Jiang Yan Xu wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, lines 267-269 https://reviews.apache.org/r/37236/diff/2/?file=1036681#file1036681line267 mesos.proto documentation on Volume::container_path and Volume::host_path both require them to be Absolute paths, should it be changed? I think we should remove that documentation in mesos.proto since different containerizer supports different cases. On Aug. 11, 2015, 9:06 p.m., Jiang Yan Xu wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, lines 312-314 https://reviews.apache.org/r/37236/diff/2/?file=1036681#file1036681line312 Are these constraints documented somewhere else that the users could more easily see? mesos.proto seems the reasonable place (aside from a user doc, of course) but I didn't find it there. We definitely need a user doc for users to understand how to use the filesystem isolator +1 - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37236/#review94832 --- On Aug. 10, 2015, 10:13 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37236/ --- (Updated Aug. 10, 2015, 10:13 p.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added the linux filesystem isolator. Note that the persistent volume part (i.e., update) hasn't been implemented yet. This review is derived from https://reviews.apache.org/r/36429/ Tests are in the subsequent review. Diffs - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 2cbb879888baf6aff76fbd7c1e19027300fb86e3 Diff: https://reviews.apache.org/r/37236/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95046 --- src/slave/containerizer/isolators/filesystem/linux.cpp (line 399) https://reviews.apache.org/r/37330/#comment149834 Can we also log the container id and it's the linux filesystem isolator? It's much easier to debug instead of just seeing a message says Unknown container. src/slave/containerizer/isolators/filesystem/linux.cpp (line 412) https://reviews.apache.org/r/37330/#comment149835 I don't think we hold reference to tempoaries? - Timothy Chen On Aug. 11, 2015, 1:57 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 11, 2015, 1:57 a.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37377: Disallow HTTP schedulers when authentication is required.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/#review95051 --- Ship it! Ship It! - Vinod Kone On Aug. 11, 2015, 11:52 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37377/ --- (Updated Aug. 11, 2015, 11:52 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Repository: mesos Description --- Since we don't have authentication implemented yet, this disallows http schedulers when authentication is required. Diffs - src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37377/diff/ Testing --- Updated the tests. Thanks, Ben Mahler
Re: Review Request 37378: Updated http api tests to use V1 Protobufs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37378/#review95056 --- Patch looks great! Reviews applied: [37082, 37192, 37378] All tests passed. - Mesos ReviewBot On Aug. 12, 2015, 12:14 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37378/ --- (Updated Aug. 12, 2015, 12:14 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- Updated to use v1 protobufs for http api tests. Added a default conversion function for FrameworkInfo in devolve() Diffs - src/internal/devolve.hpp b9a854a5e2454a4c3f2b85032b928b2cbc0b000c src/internal/devolve.cpp be74a26e9a194a8d969c667fdc084964939dfdb7 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37378/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37303: Moved scheduler library to http
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
Thanks Anand, mostly thinking we can clean up the read logic if we have a
struct to capture the reader / decoder.
Isn't it much more simpler here? It's just a one liner if check to check if
the reader is reader is not None and != for stale reader check. Here is the
code snipped I have used:
if (!reader.isSome() || reader.get() != oldReader) {
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, lines 353-355
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line353
How about:
```
Received a '500 Internal Error' for SUBSCRIBE call: Body of request
```
Remember that we don't use periods in logging messages
Sounds good to me. I suppose we don't use periods to terminate logging messages
?
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, lines 390-397
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line390
Why does master disconnection send an Error event? This can occur if
the master crashes, fails over, etc. So the scheduler should not see an
error for this?
How else would you communicate to the scheduler that it needs to subscribe
again in case of master disconnection/failover ? Feel free to re-open in case I
missed something
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 328
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line328
Can you flip this comparison?
Why would you want to do that ? process::http::statuses[200] is a constant and
helps identify bugs like if (a = 5) by keeping the constant check on the left ?
Seems like I am missing something here
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 111
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line111
This is just for testing right? Might be nice to expose as a separate
constructor with a note that it's for testing.
Not quite. I intend to add an constructor to Mesos::Mesos i.e. the wrapper
exposed class to the user later that would be used for testing. This is just
the internal implementation class.
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 117
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line117
Hm.. can we avoid the 'headers' variable? Seems clearer to keep it
local to an individual request for now.
Why ? This is anyways just the internal implementation class. Since we have to
do that for every request we make. Why not just save it as a constant member
variable ?
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, lines 219-221
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line219
This is required, so how about just saying:
```
// Each subscription requires a new connection.
disconnect();
```
Sounds good. Done
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/common/http.hpp, lines 80-81
https://reviews.apache.org/r/37303/diff/3/?file=1037897#file1037897line80
value.get() need to be called only if value.isSome()
Fixed , my bad.
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, lines 222-223
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line222
Can we call this `disconnect`? Any reason to not clear the reader
within the function rather than in the caller?
Fixed
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 323
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line323
Need to deal with isFailed case before you call .get() as well.
Fixed
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 329
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line329
CHECK_EQ?
Fixed
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 435
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line435
Can we call this 'disconnect'?
Fixed
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 437
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line437
space here :)
I wish we as a community can get adoption for clang format soon. These things
should be handled by a tool rather then someone spending time reviewing them :(
On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
src/scheduler/scheduler.cpp, line 439
https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line439
Can we avoid saying reader in these messages? Let's just say that the
connection was already closed, since the users of this library don't care
about the implementation detail of there being a Reader.
Fixed
On
