Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 2:06 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs (updated) - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 17, 2015, 7:13 a.m., haosdent huang wrote: src/tests/utils.hpp, line 72 https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line72 How about check path length and use `path[0]` here? Instead of `*(path.begin())` I think we can use startsWith to check it. On July 17, 2015, 7:13 a.m., haosdent huang wrote: src/tests/utils.hpp, line 69 https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line69 Indent also not correct here. And if `net::getHostname(process.self().address.ip)` return error, does it make incorrect url? Yes, agree with that. If we want to build a new function, Trystd::string should be the type of return value. On July 17, 2015, 7:13 a.m., haosdent huang wrote: src/tests/utils.hpp, line 55 https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line55 Because we use We use lowerCamelCase for function names (Google uses mixed case for regular functions; and their accessors and mutators match the name of the variable). So maybe need to change it like this: ``` template class T std::string endpointUrl( process::ProcessT process, const std::string path, ) ``` The indent also break the style guild rule here. Agree. On July 17, 2015, 7:13 a.m., haosdent huang wrote: src/tests/utils.hpp, line 60 https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line60 I still suggest use string method and remove cstring Yes; if we can remove it because of endsWith util fuction. On July 17, 2015, 7:13 a.m., haosdent huang wrote: src/tests/utils.hpp, line 64 https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line64 We have endsWith method in strings.hpp. Maybe use could use it. Also the style looks not correct here. Maybe need chang it like this: ``` if (protocal.length() = len || ``` Thanks; just found endsWith in our source code. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92033 --- On July 17, 2015, 2:06 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 2:06 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92082 --- Ship it! Ship It! src/tests/fetcher_tests.cpp (line 22) https://reviews.apache.org/r/36501/#comment146021 Need sort it lexicographically. ``` #include sstream #include string src/tests/fetcher_tests.cpp (line 319) https://reviews.apache.org/r/36501/#comment146022 Not need use urlStream and import sstream here because we have stringify.hpp. Just need ``` stringify(url) ``` - haosdent huang On July 17, 2015, 2:06 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 2:06 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 17, 2015, 2:20 p.m., haosdent huang wrote: Ship It! 3rdparty/libprocess/src/tests/http_tests.cpp have a unit test case TEST(URLTest, Stringification) show how to use URL and stringify it. Maybe you could use URL according that. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92082 --- On July 17, 2015, 2:06 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 2:06 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 17, 2015, 2:20 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 22 https://reviews.apache.org/r/36501/diff/4/?file=1014441#file1014441line22 Need sort it lexicographically. ``` #include sstream #include string ``` #include sstream #include string ``` Sorry for forgot type ``` - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92082 --- On July 17, 2015, 2:06 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 2:06 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/#review92031 --- Patch looks great! Reviews applied: [36461, 36462, 36463, 36464, 36465, 36466, 36467, 36469, 36470] All tests passed. - Mesos ReviewBot On July 17, 2015, 5:35 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/ --- (Updated July 17, 2015, 5:35 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/master_allocator_tests.cpp 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 Diff: https://reviews.apache.org/r/36470/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
On July 16, 2015, 7:41 p.m., Joerg Schad wrote: src/launcher/fetcher.cpp, line 134 https://reviews.apache.org/r/36547/diff/3/?file=1013405#file1013405line134 I assume those are spaces? Could you please doublecheck? These are spaces. It's probably like this to indicate that the spacing changed. On July 16, 2015, 7:41 p.m., Joerg Schad wrote: src/launcher/fetcher.cpp, line 136 https://reviews.apache.org/r/36547/diff/3/?file=1013405#file1013405line136 can you add a short comment why we assume this here? Added a check to narrow the supported URI protocols. - Jan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91913 --- On July 16, 2015, 6:55 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 6:55 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 10:32 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs (updated) - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
On July 17, 2015, 4:59 a.m., Adam B wrote: src/launcher/fetcher.cpp, lines 127-128 https://reviews.apache.org/r/36547/diff/3/?file=1013405#file1013405line127 Why not just check for any 2xx code then? Aren't they all successful in one way or another? HTTP can return 206, which means that only a part of the content has been (successfully) transferred. Therefore we should really check for the designated return codes that indicate a successful transfer of a whole resource. - Jan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92015 --- On July 16, 2015, 6:55 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 6:55 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 10:56 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Changes --- Remove redundant CHECK. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs (updated) - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92046 --- src/launcher/fetcher.cpp (line 126) https://reviews.apache.org/r/36547/#comment145935 There is an extra blank that does not belong. - Bernd Mathiske On July 17, 2015, 1:56 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 1:56 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92033 --- src/tests/utils.hpp (line 30) https://reviews.apache.org/r/36501/#comment145909 Need from the style guide: ``` #include process/process.hpp #include stout/json.hpp #include stout/option.hpp ``` src/tests/utils.hpp (line 55) https://reviews.apache.org/r/36501/#comment145910 Because we use We use lowerCamelCase for function names (Google uses mixed case for regular functions; and their accessors and mutators match the name of the variable). So maybe need to change it like this: ``` template class T std::string endpointUrl( process::ProcessT process, const std::string path, ) ``` The indent also break the style guild rule here. src/tests/utils.hpp (line 60) https://reviews.apache.org/r/36501/#comment145911 I still suggest use string method and remove cstring src/tests/utils.hpp (line 64) https://reviews.apache.org/r/36501/#comment145912 We have endsWith method in strings.hpp. Maybe use could use it. Also the style looks not correct here. Maybe need chang it like this: ``` if (protocal.length() = len || ``` src/tests/utils.hpp (line 69) https://reviews.apache.org/r/36501/#comment145913 Indent also not correct here. And if `net::getHostname(process.self().address.ip)` return error, does it make incorrect url? src/tests/utils.hpp (line 72) https://reviews.apache.org/r/36501/#comment145914 How about check path length and use `path[0]` here? Instead of `*(path.begin())` - haosdent huang On July 16, 2015, 5:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 5:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 10:40 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs (updated) - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92042 --- Ship it! Ship It! - Joerg Schad On July 17, 2015, 8:40 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 8:40 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36424: Created a command executor helper method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/#review92038 --- 3rdparty/libprocess/include/process/subprocess.hpp (lines 302 - 304) https://reviews.apache.org/r/36424/#comment145922 You may ignore this, but I'm not sure if ignoring `cerr` when the command succeds is the way to go. Most logging systems log unto the `clog` which is mostly an alias for the `cerr` file descriptor. 3rdparty/libprocess/include/process/subprocess.hpp (line 307) https://reviews.apache.org/r/36424/#comment145919 I think you should calle it `execute` to follow our concise naming. Moreover, under the context is hard to misunderstand it. 3rdparty/libprocess/include/process/subprocess.hpp (line 314) https://reviews.apache.org/r/36424/#comment145929 ranged based for loops are not yet whitelisted, nor there are any occurrence on the codebase. So I guess it is not yet time to use them. 3rdparty/libprocess/include/process/subprocess.hpp (lines 334 - 335) https://reviews.apache.org/r/36424/#comment145927 I'm not very sure about doing the `.get()`, since we didn't verify that the objects where properly constructed (in line 320) or checking that they are not `None()`. - Alexander Rojas On July 17, 2015, 7:52 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 17, 2015, 7:52 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830 and MESOS-2834 factoring out the remote command execution logic. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92045 --- Ship it! Ship It! - Joerg Schad On July 17, 2015, 8:56 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 8:56 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 11:05 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs (updated) - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92048 --- Ship it! Ship It! - Joerg Schad On July 17, 2015, 9:05 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 9:05 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36450: Introduced Address and URL protobufs.
On July 15, 2015, 7:08 p.m., Vinod Kone wrote: src/common/type_utils.cpp, line 131 https://reviews.apache.org/r/36450/diff/2/?file=1011909#file1011909line131 Is the order of query parameters important? Aren't these URLs equivalent? http://a.b.c/?k1=ak2=b http://a.b.c/?k2=bk1=a Ben Mahler wrote: Java's URI class considers ordering as important: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29 I'd also like to keep it simple for now, you'll notice that they consider percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we can just avoid this for now: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29 Ideally, we'd have a URI / URL class in stout, where we can have a comprehensive equality operator. Anand Mazumdar wrote: +1 There is already a URL struct that exists in libprocess that we might consider enriching upon later and also move it to stout. Ben Mahler wrote: I'll add a TODO related to this. Interestingly enough, the original [RFC 3986](https://tools.ietf.org/html/rfc3986) (also mentioned [here](https://en.wikipedia.org/wiki/URL_normalization)) does not specify anything, leaving the matter to determination of equivalence or difference of URIs is based on string comparison, perhaps augmented by reference to additional rules provided by URI scheme definitions. (Sec. 6.1 Equivalence) And the HTTP scheme, does not further clarify the matter; however, the only real difference seem to pertain to repeated query parameters, where the order *may* matter. For my own curiosity, as the references were to Open JDK 6, I tried it also with the Oracle JDK 8 and they still made a string-wise comparison: ``` URI uri = new URI(http://a.b.com?a=foob=bar;); URI another = new URI(http://a.b.com?b=bara=foo;); System.out.println(String.format(%s %s equal to %s, uri, uri.equals(another) ? is : is not, another)); ``` yields: ``` http://a.b.com?a=foob=bar is not equal to http://a.b.com?b=bara=foo ``` I'm just noting this, because if we do decide (as it would seem reasonable and de facto adopted) to consider two URIs equivalent (regardless of the query params ordering) this should probably be noted in the equality operator documentation (to avoid someone tripping unwittingly on it). Please be aware that URI is **different** from URL (the latter is a subset of the former). - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review91795 --- On July 17, 2015, 1:36 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto d2f482668e671b30f2586f6aae9c20132ab4d1e4 include/mesos/type_utils.hpp eb7fe2562cfcff52288d1c216425068d1ba551c0 src/common/type_utils.cpp 2ad5b4cbe324c83e81fd7df7430652f5c0a4e30f src/master/master.cpp 082758ef54597ad32cf0d025c147f0f44dd11961 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92093 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 17, 2015, 2:06 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 2:06 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 4:13 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs (updated) - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92051 --- Patch looks great! Reviews applied: [36547] All tests passed. - Mesos ReviewBot On July 17, 2015, 9:05 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 9:05 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36049: Added support for modularized Authorizer
On July 8, 2015, 12:12 a.m., Till Toenshoff wrote:
src/local/local.cpp, line 241
https://reviews.apache.org/r/36049/diff/7/?file=1000249#file1000249line241
I am assuming that the `LocalAuthorizer` should be considered unusable
should its initialize function ever fail.
My most favored solution here would be to log the failure and make sure
that `authorizer` remains unset so that we can operate without any
authorization. That would be following the approach of the authenticator
`initialize` failure handling.
```
TryNothing initialize =
authorizer.get()-initialize(flags.acls.get());
if (initialize.isError()) {
// A failure to initialize the authorizer does lead to unusable
authorization
// but allows actions to skip authorization.
LOG(WARNING) Authorization is disabled: Failed to initialize '
flags.authorizers ' authorizer:
initialize.error();
delete authorizer.get();
authorizer = None();
}
```
Inherited from
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L484
As a note, please don't use links to the master branch, use a specific review
since any update on the master invalidates the line you want to show. e.g.
https://github.com/apache/mesos/blob/bfe6c07b79550bb3d1f2ab6f5344d740e6eb6f60/src/master/master.cpp#L484
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36049/#review90774
---
On July 7, 2015, 9:34 a.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36049/
---
(Updated July 7, 2015, 9:34 a.m.)
Review request for mesos, Adam B and Till Toenshoff.
Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947
Repository: mesos
Description
---
Adds and integrates helper classes needed to support an `Authorizer` module.
Also adds a flag to the master, allowing the selection of an `Authorizer`
module.
Diffs
-
include/mesos/authorizer/authorizer.hpp PRE-CREATION
include/mesos/module/authorizer.hpp PRE-CREATION
src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851
src/authorizer/authorizer.cpp PRE-CREATION
src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff
src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce
src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341
src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f
src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756
src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9
src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670
src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26
Diff: https://reviews.apache.org/r/36049/diff/
Testing
---
make check
Thanks,
Alexander Rojas
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92052 --- Ship it! Ship It! - Bernd Mathiske On July 17, 2015, 2:05 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 17, 2015, 2:05 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92104 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 17, 2015, 4:13 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 4:13 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 17, 2015, 2:20 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 22 https://reviews.apache.org/r/36501/diff/4/?file=1014441#file1014441line22 Need sort it lexicographically. ``` #include sstream #include string haosdent huang wrote: ``` #include sstream #include string ``` Sorry for forgot type ``` No need sstream here. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92082 --- On July 17, 2015, 4:13 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 4:13 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review91503 --- src/slave/containerizer/mesos/containerizer.cpp (line 65) https://reviews.apache.org/r/34142/#comment144997 Probably left over from a previous review? src/slave/containerizer/provisioners/appc.hpp (line 54) https://reviews.apache.org/r/34142/#comment144958 Who outside appc.cpp uses it? src/slave/containerizer/provisioners/appc.hpp (line 56) https://reviews.apache.org/r/34142/#comment144959 I see that this is used by Store. Possible to move to the Store base class to break circular dependency? src/slave/containerizer/provisioners/appc.hpp (lines 58 - 61) https://reviews.apache.org/r/34142/#comment144938 Looks like this is only used in discovery. Can it be moved to Discovery so we can break the Dicovery - Appc - Discovery review dependency? src/slave/containerizer/provisioners/appc.hpp (lines 63 - 67) https://reviews.apache.org/r/34142/#comment144939 Move to cpp if this is only used by appc.cpp? src/slave/containerizer/provisioners/appc.hpp (lines 64 - 67) https://reviews.apache.org/r/34142/#comment144940 Indentation. src/slave/containerizer/provisioners/appc.hpp (line 93) https://reviews.apache.org/r/34142/#comment144933 explicit src/slave/containerizer/provisioners/appc.hpp (line 101) https://reviews.apache.org/r/34142/#comment144936 Move to cpp? src/slave/containerizer/provisioners/appc.hpp (line 125) https://reviews.apache.org/r/34142/#comment145668 According the spec the ordering is significant so we better document it here. In fact the implementation does return the list of image in **topological** order (dependencies go before dependents is the list), which is great. It doesn't address duplicates though, which is something we can improve. src/slave/containerizer/provisioners/appc.hpp (line 127) https://reviews.apache.org/r/34142/#comment145221 s/hash/id/ ? As I commented on another review, I think it's less error-prone if we consistently use `id` rather than `hash`. src/slave/containerizer/provisioners/appc.hpp (line 132) https://reviews.apache.org/r/34142/#comment145222 s/hash/id/ ? src/slave/containerizer/provisioners/appc.hpp (line 148) https://reviews.apache.org/r/34142/#comment145208 What are additional fileds you imagine may be added in the future? src/slave/containerizer/provisioners/appc.cpp (line 56) https://reviews.apache.org/r/34142/#comment144957 Such as checking `if(acKind == ImageManifest)`? src/slave/containerizer/provisioners/appc.cpp (line 84) https://reviews.apache.org/r/34142/#comment145514 IIUC about the spec there is not a canonical name but this is a template for SimpleDiscovery. e.g. MetaDiscovery, if one were to implement it, doesn't follow this. Can we move it to SimpleDisovery and make LocalDiscovery (which may be more aptly named LocalSimpleDiscovery?) a subclass of SimpleDiscovery? src/slave/containerizer/provisioners/appc.cpp (line 113) https://reviews.apache.org/r/34142/#comment145519 Given the TODO above I am not sure if we still should do this at all. Maybe it's sufficient to just fill in the missing arch label and trust the operator will do the right thing? src/slave/containerizer/provisioners/appc.cpp (lines 287 - 288) https://reviews.apache.org/r/34142/#comment145564 Inline this? Moreover, with the path manipulation getting hairy in the code (different places remove and append rootfs) I think we'd better do something akin to **slave/paths.hpp**. src/slave/containerizer/provisioners/appc.cpp (lines 322 - 323) https://reviews.apache.org/r/34142/#comment145930 This is probably where the lambda becomes to long and a callback with a descriptive name (the old style) is better? src/slave/containerizer/provisioners/appc.cpp (lines 342 - 343) https://reviews.apache.org/r/34142/#comment145762 We typically break after the operator I think :) (e.g. https://github.com/apache/mesos/blob/bfe6c07b79550bb3d1f2ab6f5344d740e6eb6f60/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp#L494) src/slave/containerizer/provisioners/appc.cpp (lines 349 - 351) https://reviews.apache.org/r/34142/#comment145253 2 spaces src/slave/containerizer/provisioners/appc.cpp (lines 365 - 375) https://reviews.apache.org/r/34142/#comment145464 Indentation: it should be two spaces right of the dot on `.then()`. src/slave/containerizer/provisioners/appc.cpp (line 409) https://reviews.apache.org/r/34142/#comment145231 Misaligned. src/slave/containerizer/provisioners/appc.cpp (line 423) https://reviews.apache.org/r/34142/#comment145463 fetchAll() already recursively traverses the dependency graph right? src/slave/containerizer/provisioners/appc.cpp (line 424)
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92108 --- src/tests/fetcher_tests.cpp (line 22) https://reviews.apache.org/r/36501/#comment146048 Because use stringify, could remove it now. src/tests/fetcher_tests.cpp (line 66) https://reviews.apache.org/r/36501/#comment146047 Because use stringify, could remove it now. - haosdent huang On July 17, 2015, 4:13 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 17, 2015, 4:13 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36450: Introduced Address and URL protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review92118 --- Ship it! Ship It! - Vinod Kone On July 17, 2015, 1:36 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto d2f482668e671b30f2586f6aae9c20132ab4d1e4 include/mesos/type_utils.hpp eb7fe2562cfcff52288d1c216425068d1ba551c0 src/common/type_utils.cpp 2ad5b4cbe324c83e81fd7df7430652f5c0a4e30f src/master/master.cpp 082758ef54597ad32cf0d025c147f0f44dd11961 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review91989 --- Not necessary to be done in this review but we should allow `putting` images that are already in the staging folder, this will support mechanisms that download stuff out-of-band. Also we should rely on a `paths.hpp` when doing all the path manipulation. src/slave/containerizer/provisioners/appc/store.hpp (line 50) https://reviews.apache.org/r/34140/#comment145928 It's worth noting that the id is computed from the sha512 hash here as this is actually the only place where the mechanism by which that this id is derived matters. Everywhere else it should be understood as an opaque string. src/slave/containerizer/provisioners/appc/store.hpp (lines 80 - 83) https://reviews.apache.org/r/34140/#comment145766 Who uses this? src/slave/containerizer/provisioners/appc/store.cpp (line 148) https://reviews.apache.org/r/34140/#comment146052 Here if the `stage` directory is moved into the store then `rmdir()` returns Error. Of course we don't check the result but maybe checking its existence before rmdir and log other rmdir Errors is better? src/slave/containerizer/provisioners/appc/store.cpp (line 397) https://reviews.apache.org/r/34140/#comment145936 So at this point we have already downloaded the image, we might as well just go ahead and overwrite the existing folder. If not, we probably don't want to spend all the resources to download it in the first place (i.e. directly return in `put()`). But I think it would make sense to overwrite if we are going to open up some HTTP API to allow operators to resue some faulty images. src/slave/containerizer/provisioners/appc/store.cpp (lines 406 - 415) https://reviews.apache.org/r/34140/#comment145933 Manifest validation is only one type of validation. We should also do other validation checks. e.g. whether rootfs exists; whether there are additional files outside of rootfs, etc. - 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/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/#review92119 --- Ship it! Ship It! - Vinod Kone On July 17, 2015, 1:36 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 and MESOS-3012 https://issues.apache.org/jira/browse/MESOS-2910 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- This relies on 'Offer.url' to compute the pids needed for the message passing optimization (see [MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)). Diffs - src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 src/tests/scheduler_event_call_tests.cpp cf6aa19a644580ff9d805e919763e9342d72677f Diff: https://reviews.apache.org/r/36498/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 34142: AppC provisioner.
On July 2, 2015, 1:48 a.m., Timothy Chen wrote: include/mesos/mesos.proto, line 1300 https://reviews.apache.org/r/34142/diff/2/?file=989783#file989783line1300 I believe we discussed this, but different acVersion will most likely have different schema. Unless we intend to only support one version (or anything that's backward compatible), we should version the protobuf too. Timothy Chen wrote: Actually at this point seems like AppC image spec most likely won't have a new version with OCP, so perhaps we don't have to. I'll let you decide :) We should at least say in the comment what acVersion this proto definiton is based on. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review90204 --- 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 34137: Add support for container image provisioners.
On July 16, 2015, 6:36 p.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, line 630 https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line630 Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is called. In general, I think we should avoid using [=] for lambdas because its dangeous! I would prefer we resort to our old style defer style (e.g., introduce `_provision`). [=] captures slaveId by value (copy) so it won't be invalid? But after when to use lambdas, I think this is a good point and we should establish some best practices. Google style guide has these guidelines: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions Avoiding default captures is one of them; limiting the length of lambdas is another. We should document these, at least reference Google's. And how about lambdas that simply call another method vs. bind? :) - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92000 --- On July 11, 2015, 9:47 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 11, 2015, 9:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34141: AppC provsioning backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34141/#review91685 --- src/slave/containerizer/provisioners/appc/backend.hpp (line 41) https://reviews.apache.org/r/34141/#comment145761 In terms of the name `Backend`, I am not sure if it captures what it does precisely. There is Provisioner::provision() and there is Backend::provision(). If provision means the overarching action then this specific step is really installing the downloaded images. Thoughts? src/slave/containerizer/provisioners/appc/backend.cpp (lines 128 - 133) https://reviews.apache.org/r/34141/#comment145250 For this to work, should we check the pre-condition: `directory` has to already exist, otherwise `rootfs` is copied to `directory` rather than `directory/rootfs`. I assume this is what we want given the bind mount backend implemetation: A simple illustration of directory layout (in `paths.hpp`) is hugely helpful. - 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/34141/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Simple copy backend that forms the rootfs by copying each layer. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34141/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36562: Store MasterInfo instead of UPID in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36562/#review92115 --- Ship it! Ship It! - Vinod Kone On July 17, 2015, 1:36 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36562/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See https://reviews.apache.org/r/36497/ for context. Diffs - src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 Diff: https://reviews.apache.org/r/36562/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36497/#review92116 --- Ship it! src/sched/sched.cpp (line 453) https://reviews.apache.org/r/36497/#comment146054 there is no master UPID anymore. update the comment? src/sched/sched.cpp (line 465) https://reviews.apache.org/r/36497/#comment146055 s/required/relied/ ? - Vinod Kone On July 17, 2015, 1:36 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36497/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case. We now need to store the MasterInfo coming from the detector, as it is not provided in Event. Diffs - src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 src/tests/scheduler_event_call_tests.cpp cf6aa19a644580ff9d805e919763e9342d72677f Diff: https://reviews.apache.org/r/36497/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 12:47 p.m.) Review request for mesos, Adam B and Jie Yu. Changes --- Addressed Jie's comments Repository: mesos Description --- Added first draft of oversubscription user doc Diffs (updated) - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/common/protobuf_utils.hpp, lines 78-79
https://reviews.apache.org/r/36318/diff/2/?file=1003766#file1003766line78
please use javadoc format
also unnecessary semicolon
s/a/an
(eg an Event)
Marco Massenzio wrote:
any particular reason for not documenting this method in accordance w/
style guide (javadoc)?
Yes, from the style guide :
Doxygen documentation needs only to be applied to source code parts that
constitute an interface for which we want to generate Mesos API documentation
files. Implementation code that does not participate in this should still be
enhanced by source code comments as appropriate, but these comments should not
follow the doxygen style.
These methods are not related to any public API and hence have non doxygen
based comments. I fixed the type you mentioned though.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 307
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line307
while you are at it, do you mind adding javadoc doxy documentation to
this method?
what it does, what the @param's are, what does it return; maybe a link
to the design doc...
as much as you feel like, really: like money and beauty, there's no too
much documentation :)
Anand Mazumdar wrote:
There is already a TODO on the CALL_HELP variable for documenting this
better. This can easily be pursued as part of that. Do you still want me to
pursue this as part of this review ?
Marco Massenzio wrote:
Yes, please - the more documentation we add, the less we avoid repeating
the effort (that you must have just done) of reverse-engineering the code and
understanding what it does.
Like money and beauty, there is no such thing as too much documentation :)
Same as above, not an public API method.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 311-316
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line311
unless you know for a fact that none of this will be `None()` you
*must* check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
return BadRequest(Content-Type header MUST be specified);
} else if (contentType.get() == Constants.APPLICATION_JSON) {
return MediaNotSupported(We do not support JSON request/response
content yet);
} else {
...
}
```
Isabel Jimenez wrote:
Hi @marco I commented on previous review that this valdiations will be
handle in the split of the /call patch. That's why it's missing here. It'll
be added with the rebase.
Marco Massenzio wrote:
I'm not entirely sure I understand (but I don't really need to): please
then add either a TODO to check the checks (so to speak) or an inline comment
stating the invariant - but, if so, I would like to see a CHECK_SOME() to
make it explicit.
Otherwise, someone else (yourself in 3 months!) may not know/remember
about this and may add redundant checks and/or or remove the others (and
leave the condition unchecked).
Anand Mazumdar wrote:
My bad, I should have added a better TODO instead of the vague one that
says Fix logic when we start supporting application/json. I will add a
better TODO. I was under the impression that all this would be taken care as
part of the validations patch.
Anand Mazumdar wrote:
Added a TODO for validation.
Marco Massenzio wrote:
ok - then please 'drop' the issue (or it looks like you haven't addressed
yet).
Had already added a TODO for validation.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 317
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line317
the error is actually 415 Media Not Supported (I think - please double
check)
Isabel Jimenez wrote:
Same here
Anand Mazumdar wrote:
Re-iterating my earlier comments, all of this needs to be handled as part
of a separate validation patch, this patch just has the minor objective of
getting subcribed events to work and not handle validations.
Marco Massenzio wrote:
same as above, then - add a TODO drop issue
Had added a TODO for validation.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 342
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line342
we should return a `BadRequest` here, shouldn't we? or use
UNREACHABLE() (but that would seem too radical to me: one could crash Mesos
with a malformed request: yay for DOS :)
Anand Mazumdar wrote:
Same as above.
Marco Massenzio wrote:
same comment
Added a generic TODO for that this would be handled as part of validation.
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/master.hpp, line 353
Re: Review Request 36424: Created a command executor helper method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 18, 2015, 2 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett. Changes --- Forgotten to convert comments to javadoc format. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830 and MESOS-2834 factoring out the remote command execution logic. Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36501: MESOS-3023
On 七月 17, 2015, 5:39 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 66 https://reviews.apache.org/r/36501/diff/5/?file=1014595#file1014595line66 Because use stringify, could remove it now. yes. agree. i'll update the code. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92108 --- On 七月 17, 2015, 4:13 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated 七月 17, 2015, 4:13 p.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review92161 --- Ship it! Ship It! - Marco Massenzio On July 15, 2015, 4:56 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 15, 2015, 4:56 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change lays the ground-work for the master's ability to stream events back to the client. This review turned out a bit too large for my own liking but in a nutshell, it just takes a subscribe request and puts a subscribed event back on the stream. Explanation of changes: - Made a generic FrameworkDriver interface that the master now uses to communicate with the frameworks instead of just invoking send(framework-pid,...) - FrameworkDriver can be of 2 types http, libprocess. An Optional member variable is used to distinguiush between them. - This still uses hard-coded http related constants. They can go away when Isabel submits her validation change (36217) - This change prefers use of using trailing under-scores as member variables from the style guide. Diffs - src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 9ac81c38efd70f92c64a5865fa79fe516e84dd92 src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + a simple test for subscribe call received a subscribed event back on the stream. Thanks, Anand Mazumdar
Re: Review Request 36424: Created a command executor helper method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 18, 2015, 1:55 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett. Changes --- First pass, after initial comments, refactoring to emit a CommandResult. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830 and MESOS-2834 factoring out the remote command execution logic. Diffs (updated) - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Review Request 36575: Added Labels to TaskStatus protobuf and expose them via state.json.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36575/ --- Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen. Bugs: MESOS-3076 https://issues.apache.org/jira/browse/MESOS-3076 Repository: mesos Description --- The labels would allow executors and Slave modules to pass in some meta-data about the task to the framework and Mesos-DNS (via state.json). Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 src/common/protobuf_utils.cpp 9ac81c38efd70f92c64a5865fa79fe516e84dd92 src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc src/tests/slave_tests.cpp 89cc7f68b33b037626ca6056647c360b5a6d5901 Diff: https://reviews.apache.org/r/36575/diff/ Testing --- make check with new tests to verify state.json output. Thanks, Kapil Arya
Re: Review Request 36586: Updated scheduler driver to send SUBSCRIBE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/ --- (Updated July 17, 2015, 11:08 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- See summary. Diffs (updated) - src/sched/sched.cpp 25e2d660f4ee4c0b21c887f78ad04819012966f9 src/tests/master_tests.cpp 1e934c4b168a0afabd5065e2c8ffa131362ed29b src/tests/rate_limiting_tests.cpp 6a93df086bc0f256c7750d06f950d61f2dfb7b5c src/tests/slave_recovery_tests.cpp de2fc280abae98a5fbbeae6e230a1bfdaf0fc86e Diff: https://reviews.apache.org/r/36586/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/#review92126 --- Bad patch! Reviews applied: [36488] Failed command: ./support/apply-review.sh -n -r 36488 Error: 2015-07-17 21:07:41 URL:https://reviews.apache.org/r/36488/diff/raw/ [10514/10514] - 36488.patch [1] error: missing binary patch data for 'docs/images/oversubscription-overview.jpg' error: binary patch does not apply to 'docs/images/oversubscription-overview.jpg' error: docs/images/oversubscription-overview.jpg: patch does not apply Failed to apply patch - Mesos ReviewBot On July 17, 2015, 8 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 8 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36585: Exposed `docker inspect` output via state.json
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36585/
---
(Updated July 17, 2015, 6:06 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Timothy Chen.
Bugs: MESOS-3061
https://issues.apache.org/jira/browse/MESOS-3061
Repository: mesos
Description (updated)
---
This would allow Mesos-DNS to lookup container information such as its
IP address.
NOTE: Apparently, the data size is about 15KB per `docker inspect` blob and
that might addup for large clusters. One suggestion is to expose only the IP
field(s) from:
NetworkSettings: {
Bridge: ,
EndpointID:
c9d8bfb120503eda3dc88ffdcc5778cff4e7f05bb2e53f9b2fba82972ecfe3da,
Gateway: 172.17.42.1,
GlobalIPv6Address: ,
GlobalIPv6PrefixLen: 0,
HairpinMode: false,
IPAddress: 172.17.0.5,
IPPrefixLen: 16,
IPv6Gateway: ,
LinkLocalIPv6Address: ,
LinkLocalIPv6PrefixLen: 0,
MacAddress: 02:42:ac:11:00:05,
NetworkID:
f36472f7725507d90ab10215d35d020bf3eaf5ebdc024cb5b3773967d9fc,
PortMapping: null,
Ports: {},
SandboxKey: /var/run/docker/netns/3c3d4d0f5e18,
SecondaryIPAddresses: null,
SecondaryIPv6Addresses: null
},
Diffs
-
src/docker/executor.cpp cdcd8ee7ad0b53748819bc1e565f708e30e99a5d
src/tests/docker_containerizer_tests.cpp
6c6f4b7f30cec9b5bba77234b714e96289c82b43
Diff: https://reviews.apache.org/r/36585/diff/
Testing
---
sudo make check
Thanks,
Kapil Arya
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 3:27 p.m.) Review request for mesos, Adam B and Jie Yu. Changes --- Addressed Jie and Adam's comments Bugs: MESOS-3033 https://issues.apache.org/jira/browse/MESOS-3033 Repository: mesos Description --- Added first draft of oversubscription user doc Diffs (updated) - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36488: Added oversubscription user doc.
On July 17, 2015, 1:37 p.m., Adam B wrote: docs/oversubscription.md, lines 99-104 https://reviews.apache.org/r/36488/diff/2-5/?file=1011899#file1011899line99 Did this removed text go somewhere else? I can't find it anymore. Seemed useful. Jie pointed out that the allocator merges them. This changed from the first designs, so I shouldn't have included it. - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/#review92122 --- On July 17, 2015, 3:27 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 3:27 p.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3033 https://issues.apache.org/jira/browse/MESOS-3033 Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36424: Created a command executor helper method.
On July 14, 2015, 12:20 a.m., Paul Brett wrote:
Artem Harutyunyan wrote:
a drive by comment suggested by Joris is inline really necessary here?
well, it is - as this is a header file (not having the `inline` will cause a
linker error for a 'duplicate definition' or something).
Happy to move it to a `.cpp` file, but there's been disagreement about this in
the past, so I'm following the same pattern as `subprocess` here.
On July 14, 2015, 12:20 a.m., Paul Brett wrote:
3rdparty/libprocess/include/process/subprocess.hpp, line 307
https://reviews.apache.org/r/36424/diff/1/?file=1008891#file1008891line307
How about returning a tuple/struct of stdout, stderr and return code
and let the caller decide what they want?
Marco Massenzio wrote:
sure, that would be a possibility too, but it seems to me that the
approved way in Mesos is to return a `Try` for when something *may* go
wrong.
This is consistent across the entire code base.
Artem Harutyunyan wrote:
Maybe I am missing something, I did a grep for `FutureTry..` and could
not find any occurence of it in the code base. Perhaps the reason is that
it's customary to use Future's `Failure()` to indicate an error (as opposed
to returning a `Try`). If anything `Result` would probably be more
appropriate here than Try, but I'd like to hear what a shepherd has to say.
The function could just return `Futurestd::string` and you could use
`Failure()` to indicate the error. In that case you'll need to change the
return type of `.then` lamda to `Futurestd::string` and also to replace a
`return Error(...` on line 346 with `return Failure(...` (which you might
want to do anyway for the sake of consistency).
Marco Massenzio wrote:
I'd like to hear what a shepherd has to say.
eh eh, no kidding - while writing this code, I swear my brain melted :)
The one thing to bear in mind (and that's probably the reason this is a
bit of a 'only child') is that the 'error mode' here is different than
elsewhere: if the command 'fails' the request to run a command actually
'succeeded' - if I try to execute: 'ls -la /tmp/foo' well, the command
executes successfully, it's just that `foo` ain't there.
So, the semantics of a Future of a Try is that, yep, your request
succeeded and, yay!, your command succeeded too *or* dang! that failed and
here's the error message.
(side note: that's also the reason why I return a 200 OK from the
`/execute` endpoint, even if the command fails - your Request, nonetheless
succeeded).
But I can be convinced both ways, and return instead a `Failure(stderr)`
However, I like the tuple idea better because (a) the exit code carries
information that we'd be losing and (b) sometimes, to understand what really
went wrong, one needs both stdout **and** stderr, so I'm considering
returning a `(code, stdout, stderr)` tuple (yay! C++11 FTW)
Upon further debate (and following @BenM advice - see below) we will be
returning a `FutureCommandResultInfo` (a protobuf that will also be defined
anew in this patch) containing all the required info.
On July 14, 2015, 12:20 a.m., Paul Brett wrote:
3rdparty/libprocess/include/process/subprocess.hpp, line 309
https://reviews.apache.org/r/36424/diff/1/?file=1008891#file1008891line309
Could we drop the option and just have it default to an empty vector.
Marco Massenzio wrote:
Again, I'm trying to be consistent with the existing code base.
Very valid option, though.
I'm happy to go either way, depending on what my shepherd says :)
Yes - I found a way to deal with the original need that works well with an
empty vector.
On July 14, 2015, 12:20 a.m., Paul Brett wrote:
3rdparty/libprocess/include/process/subprocess.hpp, line 313
https://reviews.apache.org/r/36424/diff/1/?file=1008891#file1008891line313
If you just want to copy args, why not pass by value?
Marco Massenzio wrote:
not sure I fully understand the comment, anyways, here's my motivation
around this (seemingly pointless) bit of coding: the first element of the
array is treated as `argv[0]`: it is actually ignored (entirely) during the
execution, but will be used to print the 'usage' string (and/or the error
message) - so we need to pass a non-empty arg (with ideally the first element
matching the command invocation), but I wanted to avoid the caller to worry
about these details; so instead of passing:
```
executeCommand(echo, vectorstring{echo, hello, world})
```
they can just pass the command + args and be merry.
I will do that, figured out a way to deal with the issue mentioned in my
earlier comment.
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/#review91566
Re: Review Request 36585: Exposed `docker inspect` output via state.json
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36585/#review92131 --- Patch looks great! Reviews applied: [36574, 36575, 36580, 36585] All tests passed. - Mesos ReviewBot On July 17, 2015, 8:40 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36585/ --- (Updated July 17, 2015, 8:40 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allow Mesos-DNS to lookup container information such as its IP address. Diffs - src/docker/executor.cpp cdcd8ee7ad0b53748819bc1e565f708e30e99a5d src/tests/docker_containerizer_tests.cpp 6c6f4b7f30cec9b5bba77234b714e96289c82b43 Diff: https://reviews.apache.org/r/36585/diff/ Testing --- sudo make check Thanks, Kapil Arya
Review Request 36586: Updated scheduler driver to send SUBSCRIBE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp 81637961f3d931a27ac14220409571593d70b112 src/tests/master_tests.cpp 9205ec409d10fc9f7e968eed962fd9ea3c33eed5 src/tests/rate_limiting_tests.cpp 6a93df086bc0f256c7750d06f950d61f2dfb7b5c src/tests/slave_recovery_tests.cpp de2fc280abae98a5fbbeae6e230a1bfdaf0fc86e Diff: https://reviews.apache.org/r/36586/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 2:27 p.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3033 https://issues.apache.org/jira/browse/MESOS-3033 Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36586: Updated scheduler driver to send SUBSCRIBE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/#review92144 --- Patch looks great! Reviews applied: [36586] All tests passed. - Mesos ReviewBot On July 17, 2015, 11:08 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/ --- (Updated July 17, 2015, 11:08 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp 25e2d660f4ee4c0b21c887f78ad04819012966f9 src/tests/master_tests.cpp 1e934c4b168a0afabd5065e2c8ffa131362ed29b src/tests/rate_limiting_tests.cpp 6a93df086bc0f256c7750d06f950d61f2dfb7b5c src/tests/slave_recovery_tests.cpp de2fc280abae98a5fbbeae6e230a1bfdaf0fc86e Diff: https://reviews.apache.org/r/36586/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36586: Updated scheduler driver to send SUBSCRIBE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/#review92134 --- Bad patch! Reviews applied: [36586] Failed command: ./support/apply-review.sh -n -r 36586 Error: 2015-07-17 22:15:49 URL:https://reviews.apache.org/r/36586/diff/raw/ [19897/19897] - 36586.patch [1] error: patch failed: src/sched/sched.cpp:662 error: src/sched/sched.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On July 17, 2015, 9:04 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36586/ --- (Updated July 17, 2015, 9:04 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp 81637961f3d931a27ac14220409571593d70b112 src/tests/master_tests.cpp 9205ec409d10fc9f7e968eed962fd9ea3c33eed5 src/tests/rate_limiting_tests.cpp 6a93df086bc0f256c7750d06f950d61f2dfb7b5c src/tests/slave_recovery_tests.cpp de2fc280abae98a5fbbeae6e230a1bfdaf0fc86e Diff: https://reviews.apache.org/r/36586/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/common/protobuf_utils.hpp, lines 78-79
https://reviews.apache.org/r/36318/diff/2/?file=1003766#file1003766line78
please use javadoc format
also unnecessary semicolon
s/a/an
(eg an Event)
any particular reason for not documenting this method in accordance w/ style
guide (javadoc)?
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 307
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line307
while you are at it, do you mind adding javadoc doxy documentation to
this method?
what it does, what the @param's are, what does it return; maybe a link
to the design doc...
as much as you feel like, really: like money and beauty, there's no too
much documentation :)
Anand Mazumdar wrote:
There is already a TODO on the CALL_HELP variable for documenting this
better. This can easily be pursued as part of that. Do you still want me to
pursue this as part of this review ?
Yes, please - the more documentation we add, the less we avoid repeating the
effort (that you must have just done) of reverse-engineering the code and
understanding what it does.
Like money and beauty, there is no such thing as too much documentation :)
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, lines 311-316
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line311
unless you know for a fact that none of this will be `None()` you
*must* check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
return BadRequest(Content-Type header MUST be specified);
} else if (contentType.get() == Constants.APPLICATION_JSON) {
return MediaNotSupported(We do not support JSON request/response
content yet);
} else {
...
}
```
Isabel Jimenez wrote:
Hi @marco I commented on previous review that this valdiations will be
handle in the split of the /call patch. That's why it's missing here. It'll
be added with the rebase.
Marco Massenzio wrote:
I'm not entirely sure I understand (but I don't really need to): please
then add either a TODO to check the checks (so to speak) or an inline comment
stating the invariant - but, if so, I would like to see a CHECK_SOME() to
make it explicit.
Otherwise, someone else (yourself in 3 months!) may not know/remember
about this and may add redundant checks and/or or remove the others (and
leave the condition unchecked).
Anand Mazumdar wrote:
My bad, I should have added a better TODO instead of the vague one that
says Fix logic when we start supporting application/json. I will add a
better TODO. I was under the impression that all this would be taken care as
part of the validations patch.
Anand Mazumdar wrote:
Added a TODO for validation.
ok - then please 'drop' the issue (or it looks like you haven't addressed yet).
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 317
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line317
the error is actually 415 Media Not Supported (I think - please double
check)
Isabel Jimenez wrote:
Same here
Anand Mazumdar wrote:
Re-iterating my earlier comments, all of this needs to be handled as part
of a separate validation patch, this patch just has the minor objective of
getting subcribed events to work and not handle validations.
same as above, then - add a TODO drop issue
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/http.cpp, line 342
https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line342
we should return a `BadRequest` here, shouldn't we? or use
UNREACHABLE() (but that would seem too radical to me: one could crash Mesos
with a malformed request: yay for DOS :)
Anand Mazumdar wrote:
Same as above.
same comment
On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
src/master/master.hpp, line 353
https://reviews.apache.org/r/36318/diff/2/?file=1003769#file1003769line353
use javadoc instead
Anand Mazumdar wrote:
I have a general query regarding using javadoc or doxygen styled comments
in already existing files that also follow the old style comment pattern.
If you see the comments on CR : https://reviews.apache.org/r/36106
(BenM's comments ). I tend to agree with him here, we can do a later sweep of
the entire file. What do you think ?
I do disagree with punting the problem to a later time
Also adding more work on the poor soul who will have to fix the comments (it's
hardly a sweep - it requires work and understanding the method(s) + reverse
engineering them.
Let's start making things The Right Way, and then we can expand the goodness
(by all means, feel free to fix other methods' comments too - some, all or
none).
- Marco
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 1 p.m.) Review request for mesos, Adam B and Jie Yu. Changes --- Addressed comment on container becoming revocable when using a single revocable resource. Repository: mesos Description --- Added first draft of oversubscription user doc Diffs (updated) - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36580: Added TaskStatus label decorator hook for Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36580/#review92123 --- Patch looks great! Reviews applied: [36574, 36575, 36580] All tests passed. - Mesos ReviewBot On July 17, 2015, 7:56 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36580/ --- (Updated July 17, 2015, 7:56 p.m.) Review request for mesos. Repository: mesos Description --- This allows Slave modules to expose some information to the frameworks as well as Mesos-DNS via state.json. Diffs - include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db Diff: https://reviews.apache.org/r/36580/diff/ Testing --- make check with a new hook test. Thanks, Kapil Arya
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/#review92122 --- Ship it! I can't see the jpg to see how you changed it, but the text looks great to me. Just a couple of minor things. docs/oversubscription.md (line 43) https://reviews.apache.org/r/36488/#comment146059 such as cpus i.e. cpu shares, bandwidth - redundant; You should only have such as or i.e. in this phrase. docs/oversubscription.md (line 90) https://reviews.apache.org/r/36488/#comment146060 s/regular offers will just be sent/no revocable resources will be offered/ docs/oversubscription.md (lines 99 - 104) https://reviews.apache.org/r/36488/#comment146061 Did this removed text go somewhere else? I can't find it anymore. Seemed useful. - Adam B On July 17, 2015, 1 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 1 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review92128
---
Thanks Paul for taking this on!
See my detailed comments. We should first pulling the subprocess logic into
'perf::execute' first. Then, we can add version support in the subsequent
patch. I haven't looked at the version stuff yet.
src/linux/perf.cpp (line 162)
https://reviews.apache.org/r/36378/#comment146076
Kill this line.
src/linux/perf.cpp (line 163)
https://reviews.apache.org/r/36378/#comment146071
s/commandResults/Output.
src/linux/perf.cpp (line 165)
https://reviews.apache.org/r/36378/#comment146072
s/exitCode/status/
src/linux/perf.cpp (line 166)
https://reviews.apache.org/r/36378/#comment146073
s/stdOut/out/
src/linux/perf.cpp (lines 166 - 167)
https://reviews.apache.org/r/36378/#comment146075
Do you need std:: prefix in a cpp file? Can you just use 'using
std::string' in the begining?
Please do a sweep to cleanup all the occurances in this file.
src/linux/perf.cpp (line 167)
https://reviews.apache.org/r/36378/#comment146074
s/stdErr/err/
src/linux/perf.cpp (lines 171 - 173)
https://reviews.apache.org/r/36378/#comment146070
Pulling this out is great! But can we have a patch first to do this
refactor first, and then add the 'version' stuff in the second patch?
src/linux/perf.cpp (lines 171 - 173)
https://reviews.apache.org/r/36378/#comment146077
I would s/executeCommand/execute/ since you already have a 'cmd' in the
argument list.
I would rather make this function specific to perf. For example: (no need
the 'cmd')
When MESOS-3035 gets in, you can just modify perf::execute implementation
to use that instead.
```
perf::execute({--version});
perf::execute({stat, --all-cpus});
```
src/linux/perf.cpp (line 178)
https://reviews.apache.org/r/36378/#comment146080
s/execCmd/s/
src/linux/perf.cpp (lines 179 - 183)
https://reviews.apache.org/r/36378/#comment146079
Please fix the indentation.
src/linux/perf.cpp (line 181)
https://reviews.apache.org/r/36378/#comment146078
Do you need 'stdin'? Should that be Subprocess::PATH(/dev/null)?
src/linux/perf.cpp (line 189)
https://reviews.apache.org/r/36378/#comment146081
No need for this temp variable.
src/linux/perf.cpp (lines 191 - 207)
https://reviews.apache.org/r/36378/#comment146084
As Ben suggested in the email thread, you can do:
```
process::await(
perf.get().status(),
io::read(perf.get().out().get()),
io::read(perf.get().err().get()))
.then([](...) - FutureOutput { ... });
```
You don't need the lambda capture anymore if you wrote code like above.
Please take a look at
https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/posix/disk.cpp#L428
src/linux/perf.cpp (line 203)
https://reviews.apache.org/r/36378/#comment146087
I would rather let the caller dealing with extracting exit code. So
returning 'status.get()' is better.
src/linux/perf.cpp (lines 358 - 383)
https://reviews.apache.org/r/36378/#comment146089
Do you want to simplify this as well?
I think 'perf::execute' should use 'setupChild' as well. See the following
for details:
https://issues.apache.org/jira/browse/MESOS-2462
https://reviews.apache.org/r/32805/
- Jie Yu
On July 17, 2015, 1:30 a.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
---
(Updated July 17, 2015, 1:30 a.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Jie Yu, and Cong
Wang.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
---
Refactor Linux Performance monitor to handle changing 'perf stat' output
versions depending on kernel version.
Diffs
-
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
---
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review92133 --- src/linux/perf.cpp (line 178) https://reviews.apache.org/r/36378/#comment146094 Maybe 'perf' is better. - Jie Yu On July 17, 2015, 1:30 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 17, 2015, 1:30 a.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 34142: AppC provisioner.
On July 1, 2015, 5:40 p.m., Lily Chen wrote: src/slave/containerizer/provisioners/appc.cpp, lines 143-151 https://reviews.apache.org/r/34142/diff/2/?file=989787#file989787line143 What if the candidate is over-specified (has more labels)? Should this still be a match? According to the spec it should be a match. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review90173 --- 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 34140: AppC image store
On May 27, 2015, 4:10 p.m., Paul Brett wrote: src/slave/containerizer/provisioners/appc/store.cpp, line 267 https://reviews.apache.org/r/34140/diff/1/?file=957277#file957277line267 Why not do the decompress, hash untar as a pipeline to reduce disk usage? Ian Downes wrote: Because we need to hash the (decrypted (uncompressed)) tarball, i.e., an intermediate object, and because we don't know how it is compressed. It may take more effort but I think it can be done so maybe this is what we should eventually do? 1) detecting the encryption and compression type up front. 2) `tee` the decompression output to do the hash and write it to a file. For bigger images I do think the saves both temporary disk space as well as reduce latency (less disk writes). Thoughts? - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review85456 --- 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/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Review Request 36585: Exposed `docker inspect` output via state.json
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36585/ --- Review request for mesos, Benjamin Hindman, Ben Mahler, and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allow Mesos-DNS to lookup container information such as its IP address. Diffs - src/docker/executor.cpp cdcd8ee7ad0b53748819bc1e565f708e30e99a5d src/tests/docker_containerizer_tests.cpp 6c6f4b7f30cec9b5bba77234b714e96289c82b43 Diff: https://reviews.apache.org/r/36585/diff/ Testing --- sudo make check Thanks, Kapil Arya
Re: Review Request 36424: Created a command executor helper method.
On July 17, 2015, 8:48 a.m., Alexander Rojas wrote: 3rdparty/libprocess/include/process/subprocess.hpp, lines 302-304 https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line302 You may ignore this, but I'm not sure if ignoring `cerr` when the command succeds is the way to go. Most logging systems log unto the `clog` which is mostly an alias for the `cerr` file descriptor. not ignoring this at all :) In fact, we've agreed to return a `struct` with the composite info (retcode, stdout, stderr) - as you are absolutely right, sometimes even successful execution emit useful logs on stderr. On July 17, 2015, 8:48 a.m., Alexander Rojas wrote: 3rdparty/libprocess/include/process/subprocess.hpp, line 307 https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line307 I think you should calle it `execute` to follow our concise naming. Moreover, under the context is hard to misunderstand it. You are correct; however, my concern is that the verb `execute` has been vastly overloaded in the Mesos codebase, as well as being prone (here) to misunderstanding. I would prefer to keep it as is, but I'm happy to change it, if there is consensus that calling it `execute` won't cause confusion. On July 17, 2015, 8:48 a.m., Alexander Rojas wrote: 3rdparty/libprocess/include/process/subprocess.hpp, line 314 https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line314 ranged based for loops are not yet whitelisted, nor there are any occurrence on the codebase. So I guess it is not yet time to use them. Well, I am a vocal critic of whitelisting - in theory, this may mean that our style guide could potentially become as large as the ISO standard guide? :) I don't really see any value in *not* using this here: it's totally clear, it's supported by **all** the compilers/platforms we support and there is no chance of this being misinterpreted. I don't see what continuing to use something that has been, honestly, superseded 4 years ago by the C++11 standard would buy us? Also, I note in passing that your argument is [tautologic](https://en.wikipedia.org/wiki/Circular_reasoning): we should not use X because we have no occurrences of X in the code base... On July 17, 2015, 8:48 a.m., Alexander Rojas wrote: 3rdparty/libprocess/include/process/subprocess.hpp, lines 334-335 https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line334 I'm not very sure about doing the `.get()`, since we didn't verify that the objects where properly constructed (in line 320) or checking that they are not `None()`. good catch! I'll fix this. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/#review92038 --- On July 17, 2015, 5:52 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 17, 2015, 5:52 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830 and MESOS-2834 factoring out the remote command execution logic. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/#review92132 --- Ship it! LGTM! Thanks Niklas! docs/oversubscription.md (line 35) https://reviews.apache.org/r/36488/#comment146091 When the latest estimate is different from the previous estimate. docs/oversubscription.md (line 47) https://reviews.apache.org/r/36488/#comment146092 launch tasks docs/oversubscription.md (lines 194 - 197) https://reviews.apache.org/r/36488/#comment146093 Could you please align the text at 80 char width? Please do a sweep in this file. Thanks! - Jie Yu On July 17, 2015, 9:27 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 9:27 p.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3033 https://issues.apache.org/jira/browse/MESOS-3033 Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 17, 2015, 3:19 p.m.) Review request for mesos, Adam B and Jie Yu. Changes --- Updated image with binary diff Bugs: MESOS-3033 https://issues.apache.org/jira/browse/MESOS-3033 Repository: mesos Description --- Added first draft of oversubscription user doc Diffs (updated) - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
