Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 24, 2015, 9:57 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 (updated) - src/tests/fetcher_tests.cpp e2a52f7 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 20, 2015, 4:42 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 297 https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line297 According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this ``` process::http::URL url( http, process.self().address.ip, process.self().address.port, /help); ``` But I perfer chang it like this ``` const network::Address address = process.self().address; process::http::URL url(http, address.ip, address.port, /help); ``` Or add `using URL` like this ``` using process::Future; using process::http::URL; (Left a blank below and after process::Future) ``` and then ``` const network::Address address = process.self().address; URL url(http, address.ip, address.port, /help); ``` Bernd Mathiske wrote: I agree. Thanks, haosdent! I address the comments by option 2. Reguarding using URL, it'll compile because there's also mesos::URL here. But mesos::URL is a protobuf class, it will make code more complex. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 20, 2015, 4:42 p.m., haosdent huang wrote: haosdent huang wrote: Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-) Thanks very much for your patience; yes, it tooks time for me to swith between different code style :). On July 20, 2015, 4:42 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 314 https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line314 It would be better to add ```#include stout/stringify.hpp``` after ```#include stout/protobuf.hpp``` stout/stringify.hpp has been included; so did not add it. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92889 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 24, 2015, 9:57 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 24, 2015, 9:57 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 e2a52f7 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 24, 2015, 2:01 p.m., Klaus Ma wrote: If you don't get to them first, I will fix the remaining little style suggestions when committing. OK, please help to fix it. I'll learn it from the final code :). Thanks very much. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92911 --- On July 24, 2015, 9:57 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 24, 2015, 9:57 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 e2a52f7 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 20, 2015, 9:42 a.m., haosdent huang wrote: haosdent huang wrote: Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-) Klaus Ma wrote: Thanks very much for your patience; yes, it tooks time for me to swith between different code style :). Me too! On July 20, 2015, 9:42 a.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 314 https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line314 It would be better to add ```#include stout/stringify.hpp``` after ```#include stout/protobuf.hpp``` Klaus Ma wrote: stout/stringify.hpp has been included; so did not add it. No matter. Best to be explicit even if redundant, IMHO. (That's why we have provisions in headers that they only get processed once.) - Bernd --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- On July 24, 2015, 2:57 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 24, 2015, 2:57 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 e2a52f7 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/#review92911 --- Ship it! src/tests/fetcher_tests.cpp (line 295) https://reviews.apache.org/r/36501/#comment147177 It would be slightly better to separate these declarations more obviously by putting a new line here (between line 294 and 295). If you don't get to them first, I will fix the remaining little style suggestions when committing. - Bernd Mathiske On July 24, 2015, 2:57 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 24, 2015, 2:57 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 e2a52f7 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 20, 2015, 9:42 a.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 297 https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line297 According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this ``` process::http::URL url( http, process.self().address.ip, process.self().address.port, /help); ``` But I perfer chang it like this ``` const network::Address address = process.self().address; process::http::URL url(http, address.ip, address.port, /help); ``` Or add `using URL` like this ``` using process::Future; using process::http::URL; (Left a blank below and after process::Future) ``` and then ``` const network::Address address = process.self().address; URL url(http, address.ip, address.port, /help); ``` I agree. Thanks, haosdent! - Bernd --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- On July 18, 2015, 2:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 2:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- src/tests/fetcher_tests.cpp (line 297) https://reviews.apache.org/r/36501/#comment146333 According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this ``` process::http::URL url( http, process.self().address.ip, process.self().address.port, /help); ``` But I perfer chang it like this ``` const network::Address address = process.self().address; process::http::URL url(http, address.ip, address.port, /help); ``` Or add `using URL` like this ``` using process::Future; using process::http::URL; (Left a blank below and after process::Future) ``` and then ``` const network::Address address = process.self().address; URL url(http, address.ip, address.port, /help); ``` src/tests/fetcher_tests.cpp (line 314) https://reviews.apache.org/r/36501/#comment146334 It would be better to add ```#include stout/stringify.hpp``` after ```#include stout/protobuf.hpp``` - haosdent huang On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 20, 2015, 4:42 p.m., haosdent huang wrote: Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-) - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92270 --- On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs (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
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review92170 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 18, 2015, 9:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 18, 2015, 9:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- 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 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 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 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 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 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 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91871 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot 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 36501: MESOS-3023
On July 15, 2015, 4:16 a.m., haosdent huang wrote: src/tests/utils.hpp, line 55 https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line55 The code styple need change to follow https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md I've read this code style guidance and updated code accordingly :). Thanks very much for your comments. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91706 --- 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 36501: MESOS-3023
On July 15, 2015, 4:16 a.m., haosdent huang wrote: src/tests/utils.hpp, line 54 https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line54 I suggest move the implement to cpp file. As far as I known, C++ can not declare template in header file and implete it in cpp. I also have a try with the example. In C++11, it only introduced extern template to avoid duplicated template instance. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91706 --- 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 36501: MESOS-3023
--- 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 (updated) - 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 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 4:41 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 (updated) - 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 36501: MESOS-3023
On July 15, 2015, 9:19 p.m., Joseph Wu wrote:
src/tests/utils.hpp, line 55
https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line55
* Tab size = 2 spaces.
* Parameters are indented by 4 spaces.
* Comments start with a capital letter and end with a period.
* Logical blocks have the opening { on the same line.
Thanks very much for your input :). The code diff has been updated.
- Klaus
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91823
---
On July 16, 2015, 4:41 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, 4:41 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 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91863 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 16, 2015, 4:41 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, 4:41 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 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91732 --- src/tests/utils.hpp (line 26) https://reviews.apache.org/r/36501/#comment145331 Using ::strlen to get the length of ://, did not want to hardcord to 3. src/tests/utils.hpp (line 54) https://reviews.apache.org/r/36501/#comment145334 As far as I known, C++ can not declare template in header file and implete it in cpp. I also have a try with the example. In C++11, it only introduced extern template to avoid duplicated template instance. src/tests/utils.hpp (line 55) https://reviews.apache.org/r/36501/#comment145335 Do you mean ident of parameters? - Klaus Ma On July 15, 2015, 2:59 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 15, 2015, 2:59 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 ae10c420f7dddb8650377c91b5343591e8560392 src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 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 15, 2015, 2:59 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 ae10c420f7dddb8650377c91b5343591e8560392 src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 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
