Review Request 37531: MESOS-3070
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/ --- Review request for mesos. Bugs: MESOS-3070 https://issues.apache.org/jira/browse/MESOS-3070 Repository: mesos Description --- MESOS-3070 (Master CHECK failure if a framework uses duplicated task id.) Diffs - include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02 src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 src/messages/messages.proto 8977d8e0f3b16003128b6b9cab556a7b224f083c Diff: https://reviews.apache.org/r/37531/diff/ Testing --- Draft code diff, will update UT cases later. Thanks, Klaus Ma
Re: Review Request 37657: Document iptable rule need added when use docker bridge network mode.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37657/#review96020 --- docs/docker-containerizer.md (line 21) https://reviews.apache.org/r/37657/#comment151215 I'm also not a native speaker, but i'd suggest to: If *iptables* enabled on slave, make sure *iptables* accept all traffic from docker bridge interface. Example: ``` iptables -A INPUT -s 172.17.0.0/16 -i docker0 -p tcp -j ACCEPT ``` - Klaus Ma On 八月 20, 2015, 5:30 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37657/ --- (Updated 八月 20, 2015, 5:30 p.m.) Review request for mesos, Klaus Ma and Timothy Chen. Bugs: MESOS-3053 https://issues.apache.org/jira/browse/MESOS-3053 Repository: mesos Description --- Document iptable rule need added when use docker bridge network mode. Diffs - docs/docker-containerizer.md 73f897780a0bb72ab092cb08186a228e3084e798 Diff: https://reviews.apache.org/r/37657/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37531: MESOS-3070
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/ --- (Updated 八月 21, 2015, 2:10 a.m.) Review request for mesos. Bugs: MESOS-3070 https://issues.apache.org/jira/browse/MESOS-3070 Repository: mesos Description --- MESOS-3070 (Master CHECK failure if a framework uses duplicated task id.) Diffs (updated) - include/mesos/mesos.proto 33e1b28 include/mesos/type_utils.hpp dafe1df src/master/master.hpp 0432842 src/master/master.cpp 95207d2 src/master/validation.cpp ffb7bf0 src/messages/messages.proto 8977d8e src/sched/sched.cpp 012af05 src/slave/slave.cpp 2a99abc src/tests/master_tests.cpp 8a6b98b Diff: https://reviews.apache.org/r/37531/diff/ Testing --- Draft code diff, will update UT cases later. Thanks, Klaus Ma
Re: Review Request 37531: MESOS-3070
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/#review95670 --- src/master/master.cpp (line 3218) https://reviews.apache.org/r/37531/#comment150763 Yes; the TaskID will send back to framework by statusUpdate, so framework can use the UUID to kill a task which is not included in taskTag logic. - Klaus Ma On 八月 17, 2015, 1:28 p.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/ --- (Updated 八月 17, 2015, 1:28 p.m.) Review request for mesos. Bugs: MESOS-3070 https://issues.apache.org/jira/browse/MESOS-3070 Repository: mesos Description --- MESOS-3070 (Master CHECK failure if a framework uses duplicated task id.) Diffs - include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02 src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 src/messages/messages.proto 8977d8e0f3b16003128b6b9cab556a7b224f083c Diff: https://reviews.apache.org/r/37531/diff/ Testing --- Draft code diff, will update UT cases later. Thanks, Klaus Ma
Re: Review Request 37657: Document iptable rule need added when use docker bridge network mode.
On 八月 21, 2015, 1:26 a.m., Klaus Ma wrote: docs/docker-containerizer.md, line 21 https://reviews.apache.org/r/37657/diff/1/?file=1045176#file1045176line21 I'm also not a native speaker, but i'd suggest to: If *iptables* enabled on slave, make sure *iptables* accept all traffic from docker bridge interface. Example: ``` iptables -A INPUT -s 172.17.0.0/16 -i docker0 -p tcp -j ACCEPT ``` haosdent huang wrote: Because the patch is submitted, if it is not urgent, could update next time. :-) sure, it's clear enough to end user :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37657/#review96020 --- On 八月 20, 2015, 5:30 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37657/ --- (Updated 八月 20, 2015, 5:30 p.m.) Review request for mesos, Klaus Ma and Timothy Chen. Bugs: MESOS-3053 https://issues.apache.org/jira/browse/MESOS-3053 Repository: mesos Description --- Document iptable rule need added when use docker bridge network mode. Diffs - docs/docker-containerizer.md 73f897780a0bb72ab092cb08186a228e3084e798 Diff: https://reviews.apache.org/r/37657/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 36946: Factored out the pattern for URL generation in a fetcher test.
On July 30, 2015, 5:14 p.m., haosdent huang wrote: src/tests/fetcher_tests.cpp, line 332 https://reviews.apache.org/r/36946/diff/1/?file=1025117#file1025117line332 ``` // Tests whether fetcher can process URIs that contain leading whitespace ``` Does this patch broke this? Nop, it did NOT include the UT for that case (Tests whether fetcher can process URIs that contain leading whitespace). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/#review93605 --- On July 30, 2015, 4:37 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/ --- (Updated July 30, 2015, 4:37 p.m.) Review request for mesos, Bernd Mathiske and Klaus Ma. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Factored out the pattern for URL generation in (another)fetcher test. Diffs - src/tests/fetcher_tests.cpp 3ded3c037bdfe7095aa15503d81a8d2ee9d420df Diff: https://reviews.apache.org/r/36946/diff/ Testing --- GTEST_FILTER='FetcherTest.OSNetUriSpaceTest' make check Thanks, Artem Harutyunyan
Re: Review Request 37053: Set TaskStatus.executor_id on receiving a StatusUpdate from Executor.
On Aug. 4, 2015, 11:03 p.m., Niklas Nielsen wrote: src/slave/slave.cpp, lines 2720-2721 https://reviews.apache.org/r/37053/diff/2/?file=1028868#file1028868line2720 What do you think about emitting a warning if the executor id is already set? It could be, that users have been abusing this field before (I am not aware of any, but it is a possibility). If not, we should comment on it in mesos.proto. And if it's not expected behaviour, suggest to log a warning or info message. it's better to set executor id in a central place. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37053/#review94143 --- On Aug. 4, 2015, 6:32 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37053/ --- (Updated Aug. 4, 2015, 6:32 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-3196 https://issues.apache.org/jira/browse/MESOS-3196 Repository: mesos Description --- Always set executor_id when sending StatusUpdate. Diffs - src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 src/tests/master_tests.cpp 2aea430951d40d8fe78f656b1269c5e55a0802db src/tests/scheduler_tests.cpp 98fc70bf43ba99b54064a236795c7e1269004b71 Diff: https://reviews.apache.org/r/37053/diff/ Testing --- make check with a couple of added checks that verify that the executor_id is set in the incoming TaskStatus in Master as well as Scheduler. Thanks, Kapil Arya
Review Request 37168: MESOS-3063
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- Review request for mesos. Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- MESOS-3063 (Add an example framework using dynamic reservation) Diffs - src/examples/dynamic_reservation_executor.cpp PRE-CREATION src/examples/dynamic_reservation_framework.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37168/diff/ Testing --- Test Steps Build the example and run in local Mesos cluster Example Output I0806 15:19:33.955976 17033 sched.cpp:640] Framework registered with 20150805-204038-16842879-5050-14289-0015 Registered! Received offer 20150805-204038-16842879-5050-14289-O161 with mem(*):2862; disk(*):460240; ports(*):[31000-32000]; cpus(*):4 Reserve resources cpus(test, dynamic-reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Received offer 20150805-204038-16842879-5050-14289-O162 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic- reservation-framework-cpp):128 Launching task 0 using offer 20150805-204038-16842879-5050-14289-O162 Task 0 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O163 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 1 using offer 20150805-204038-16842879-5050-14289-O163 Task 1 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O164 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 2 using offer 20150805-204038-16842879-5050-14289-O164 Task 2 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O165 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 3 using offer 20150805-204038-16842879-5050-14289-O165 Task 3 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O166 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 4 using offer 20150805-204038-16842879-5050-14289-O166 Task 4 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O167 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Unreserve resources cpus(test, dynamic-reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Received offer 20150805-204038-16842879-5050-14289-O168 with mem(*):2862; disk(*):460240; ports(*):[31000-32000]; cpus(*):4 I0806 15:19:49.515024 17031 sched.cpp:1748] Asked to stop the driver I0806 15:19:49.515115 17031 sched.cpp:1032] Stopping framework '20150805-204038-16842879-5050-14289-0015' I0806 15:19:49.515630 17027 sched.cpp:1748] Asked to stop the driver 2015-08-06 15:19:49,516:17027(0x7f23f610d700):ZOO_INFO@zookeeper_close@2505: Closing zookeeper sessionId=0x34da910aab20058 to [9.111.159.76:2181] Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063
On 八月 13, 2015, 2:56 a.m., haosdent huang wrote: src/examples/dynamic_reservation_framework.cpp, line 343 https://reviews.apache.org/r/37168/diff/1/?file=1033410#file1033410line343 Does we need add ``` logging::initialize(argv[0], flags, true); ``` here? logging is an internal tools. In this example, i'm trying to avoid using internal classes; so user can build this example from installed Mesos, which is better practice for the user :). On 八月 13, 2015, 2:56 a.m., haosdent huang wrote: src/examples/dynamic_reservation_framework.cpp, line 278 https://reviews.apache.org/r/37168/diff/1/?file=1033410#file1033410line278 Could we change the style here to make it looks better? :-) sure, I copied this from other example; anyway, I'll check the style :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review95227 --- On 八月 6, 2015, 8:03 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated 八月 6, 2015, 8:03 a.m.) Review request for mesos. Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- MESOS-3063 (Add an example framework using dynamic reservation) Diffs - src/examples/dynamic_reservation_executor.cpp PRE-CREATION src/examples/dynamic_reservation_framework.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37168/diff/ Testing --- Test Steps Build the example and run in local Mesos cluster Example Output I0806 15:19:33.955976 17033 sched.cpp:640] Framework registered with 20150805-204038-16842879-5050-14289-0015 Registered! Received offer 20150805-204038-16842879-5050-14289-O161 with mem(*):2862; disk(*):460240; ports(*):[31000-32000]; cpus(*):4 Reserve resources cpus(test, dynamic-reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Received offer 20150805-204038-16842879-5050-14289-O162 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic- reservation-framework-cpp):128 Launching task 0 using offer 20150805-204038-16842879-5050-14289-O162 Task 0 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O163 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 1 using offer 20150805-204038-16842879-5050-14289-O163 Task 1 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O164 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 2 using offer 20150805-204038-16842879-5050-14289-O164 Task 2 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O165 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 3 using offer 20150805-204038-16842879-5050-14289-O165 Task 3 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O166 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Launching task 4 using offer 20150805-204038-16842879-5050-14289-O166 Task 4 is in state TASK_FINISHED Received offer 20150805-204038-16842879-5050-14289-O167 with mem(*):2734; disk(*):460240; ports(*):[31000-32000]; cpus(*):3; cpus(test, dynamic- reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Unreserve resources cpus(test, dynamic-reservation-framework-cpp):1; mem(test, dynamic-reservation-framework-cpp):128 Received offer 20150805-204038-16842879-5050-14289-O168 with mem(*):2862; disk(*):460240; ports(*):[31000-32000]; cpus(*):4 I0806 15:19:49.515024 17031 sched.cpp:1748] Asked to stop the driver I0806 15:19:49.515115 17031 sched.cpp:1032] Stopping framework '20150805-204038-16842879-5050-14289-0015' I0806 15:19:49.515630 17027 sched.cpp:1748] Asked to stop the driver 2015-08-06 15:19:49,516:17027(0x7f23f610d700):ZOO_INFO@zookeeper_close@2505: Closing zookeeper sessionId=0x34da910aab20058 to [9.111.159.76:2181] 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 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
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
--- 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
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/ --- (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 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
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/ --- (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/#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 38218: Quota: Extended the Allocator interface with quota-related methods.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38218/#review103910 --- include/mesos/master/allocator.hpp (line 358) <https://reviews.apache.org/r/38218/#comment162022> It said Mesos master will also validate the quota request in design document; should we clarify which part is covered by Mesos master and which part should be covered by allocator? For example, according to this comments, empty role will pass master's checking; it's allocator's resposibility to accept or reject it. - Klaus Ma On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38218/ > --- > > (Updated Oct. 24, 2015, 12:38 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van > Remoortere. > > > Bugs: MESOS-3716 > https://issues.apache.org/jira/browse/MESOS-3716 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 > src/master/allocator/mesos/allocator.hpp > c5375aa89b210e46c41ac7d68d119749de15d2f5 > src/master/allocator/mesos/hierarchical.hpp > cfd937ba306273c24fb5337dfeb1a15e1545169b > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a > > Diff: https://reviews.apache.org/r/38218/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38627/#review103909 --- Ship it! Ship It! - Klaus Ma On Oct. 21, 2015, 8:52 p.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38627/ > --- > > (Updated Oct. 21, 2015, 8:52 p.m.) > > > Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till > Toenshoff. > > > Bugs: MESOS-3072 > https://issues.apache.org/jira/browse/MESOS-3072 > > > Repository: mesos > > > Description > --- > > Allows developers to provide their own parameters when loading modules > instead of using the ones provided by the user when loading Mesos. This helps > to deal with default modules (those used when the user doesn't provide any), > and for testing of the modules. > > > Diffs > - > > src/examples/example_module_impl.cpp > db015cea130701a4e0a6fcb890c79fbb0c02c1ce > src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 > src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef > src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 > src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 > > Diff: https://reviews.apache.org/r/38627/diff/ > > > Testing > --- > > make check > > > Thanks, > > Alexander Rojas > >
Re: Review Request 39285: Added Quota Request Validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39285/#review103918 --- src/master/master.hpp (line 864) <https://reviews.apache.org/r/39285/#comment162026> Should we return Bad Request (404) for now, because we did not implement it yet. src/master/master.hpp (line 875) <https://reviews.apache.org/r/39285/#comment162027> Same as above. src/master/quota_handler.cpp (line 105) <https://reviews.apache.org/r/39285/#comment162034> It said the role maybe not set in allocator interface's comments (`setQuota`); so any case that the role passed validation in master but it's still empty to allocator? - Klaus Ma On Oct. 25, 2015, 3:42 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39285/ > --- > > (Updated Oct. 25, 2015, 3:42 a.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3199 > https://issues.apache.org/jira/browse/MESOS-3199 > > > Repository: mesos > > > Description > --- > > Added Quota Request Validation. > > > Diffs > - > > src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 > src/master/quota_handler.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39285/diff/ > > > Testing > --- > > > Thanks, > > Joerg Schad > >
Re: Review Request 38580: Added docker registry RemotePuller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/#review103930 --- Please link this RR to ticket. - Klaus Ma On Oct. 24, 2015, 12:57 a.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38580/ > --- > > (Updated Oct. 24, 2015, 12:57 a.m.) > > > Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. > > > Repository: mesos > > > Description > --- > > Integrated remote puller with store. Tests will follow. > > > Diffs > - > > src/CMakeLists.txt 98e76cee81ab206f3ffe7989711abc38f49c4352 > src/Makefile.am 96ce73b301c55d23bf4a5292e3d028148426a878 > src/slave/containerizer/provisioner/docker/local_puller.hpp > 4574e8a04663482625d7b54f765741f221ec13e0 > src/slave/containerizer/provisioner/docker/local_puller.cpp > 74d0e1ead7d630e65a7e75cb6123139b9197efef > src/slave/containerizer/provisioner/docker/puller.hpp > 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 > src/slave/containerizer/provisioner/docker/puller.cpp > cb05324689ffa26ce830b513e2d71b55517da3cb > src/slave/containerizer/provisioner/docker/registry_client.hpp > 1d3377e7462908246dfac90aa0c56314406529c9 > src/slave/containerizer/provisioner/docker/registry_client.cpp > 471783d88b73b62afacac3d7952ebb5d5f442097 > src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 > src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 > src/tests/containerizer/provisioner_docker_tests.cpp > 822aa77d0be0797ab62a5798527618b2cc4f6bf0 > > Diff: https://reviews.apache.org/r/38580/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39401/#review104013 --- src/master/allocator/mesos/hierarchical.cpp (line 1004) <https://reviews.apache.org/r/39401/#comment162203> Just check the code, dynamically reserved resource are included in allocation. - Klaus Ma On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39401/ > --- > > (Updated Oct. 24, 2015, 12:38 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van > Remoortere. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39401/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39401/#review103983 --- src/master/allocator/mesos/hierarchical.cpp (line 1018) <https://reviews.apache.org/r/39401/#comment162125> Can we move this out of the for loop? if the role is satisfied, framewor sort is not necessary. src/master/allocator/mesos/hierarchical.cpp (line 1056) <https://reviews.apache.org/r/39401/#comment162192> s/overcimmittment/overcommittment src/master/allocator/mesos/hierarchical.cpp (line 1094) <https://reviews.apache.org/r/39401/#comment162201> `allocable(...)` checked whether resource is enough to allocate (cpu > MIN_CPU && men > MIN_MEN). So if the resources can not allocate here, is also can not allocate in DRF stage. src/master/allocator/mesos/hierarchical.cpp (line 1101) <https://reviews.apache.org/r/39401/#comment162202> My understading of this code is to lay aside resources for unsatisfaied Quota in next allocation cycle, right? If so, I think we can move this out to another `foreach slaveIds` loop, so we did not need to lay aside resources. - Klaus Ma On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39401/ > --- > > (Updated Oct. 24, 2015, 12:38 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van > Remoortere. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39401/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39399: Quota: Refactored hierarchical allocator in preparation for quota.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39399/#review103888 --- Ship it! Ship It! - Klaus Ma On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39399/ > --- > > (Updated Oct. 24, 2015, 12:38 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van > Remoortere. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.hpp > cfd937ba306273c24fb5337dfeb1a15e1545169b > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39399/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically
> On Oct. 21, 2015, 3:02 p.m., Klaus Ma wrote:
> > src/module/manager.hpp, line 94
> > <https://reviews.apache.org/r/38627/diff/2/?file=1101800#file1101800line94>
> >
> > Should we merge with `moduleParameters[moduleName]` or replace it? IMO,
> > prefer to merge them; I'd like to leave it to you and your shepherd.
>
> Alexander Rojas wrote:
> I don't understand what you mean with this comment? is the same as the
> following one?
for example, if `{a:1, b:2}` in `moduleParameters[moduleName]` and `{b:3, c:4}`
in `params`, what's expected result? `{a:1, b:3, c:4}` or `{b:3, c:4}`.
- Klaus
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38627/#review103365
---
On Oct. 21, 2015, 8:52 p.m., Alexander Rojas wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
>
> (Updated Oct. 21, 2015, 8:52 p.m.)
>
>
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till
> Toenshoff.
>
>
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Allows developers to provide their own parameters when loading modules
> instead of using the ones provided by the user when loading Mesos. This helps
> to deal with default modules (those used when the user doesn't provide any),
> and for testing of the modules.
>
>
> Diffs
> -
>
> src/examples/example_module_impl.cpp
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce
> src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8
> src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef
> src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2
> src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50
>
> Diff: https://reviews.apache.org/r/38627/diff/
>
>
> Testing
> ---
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 39285: Added Quota Request Validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39285/#review105668 --- Ship it! Ship It! - Klaus Ma On Nov. 9, 2015, 8:51 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39285/ > --- > > (Updated Nov. 9, 2015, 8:51 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3199 > https://issues.apache.org/jira/browse/MESOS-3199 > > > Repository: mesos > > > Description > --- > > Added Quota Request Validation. > > > Diffs > - > > src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac > src/master/quota_handler.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39285/diff/ > > > Testing > --- > > make test > > > Thanks, > > Joerg Schad > >
Re: Review Request 39285: Added Quota Request Validation.
> On Nov. 7, 2015, 12:19 a.m., Klaus Ma wrote: > > src/master/quota_handler.cpp, line 140 > > <https://reviews.apache.org/r/39285/diff/7/?file=1118136#file1118136line140> > > > > Suggest to move it into the loop; if any role is not known by master, > > we did not need to continue to check others. > > Alexander Rukletsov wrote: > I think the flow is more readable how it's now: in the loop we > reconstruct the "reference" role, afterwards we check whether it is known to > the master. Also, my gut feeling is that typos in roles will not be that > frequent, so checking it once instead of per resource makes sense to me. > > Joerg Schad wrote: > There should just be a single role per request, why should I check that > in the loop? `::protobuf::parse()` will be heavy, we can end loop early if the role is not known by master. But as @AlexR said, typos in role is rare case, it also make sense to me to keep current behavior. > On Nov. 7, 2015, 12:19 a.m., Klaus Ma wrote: > > src/master/quota_handler.cpp, line 180 > > <https://reviews.apache.org/r/39285/diff/7/?file=1118136#file1118136line180> > > > > Should we move it into `validateQuotaRequest`? If any role is exist in > > master, we did not need to continue to check others. And in QuotaHandler, > > we had the pointer to master `Master* master`. > > Alexander Rukletsov wrote: > I'd keep it here, because it's related to how we currently process the > request, rather than whether the request is valid. > > Joerg Schad wrote: > There was an earlier comment from Joris mentioning that this isn't really > part of validatating the request (as it also involves state of the master). Agree :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39285/#review105440 --- On Nov. 9, 2015, 8:51 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39285/ > --- > > (Updated Nov. 9, 2015, 8:51 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3199 > https://issues.apache.org/jira/browse/MESOS-3199 > > > Repository: mesos > > > Description > --- > > Added Quota Request Validation. > > > Diffs > - > > src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac > src/master/quota_handler.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39285/diff/ > > > Testing > --- > > make test > > > Thanks, > > Joerg Schad > >
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated Nov. 9, 2015, 1:29 p.m.) Review request for mesos and Michael Park. Changes --- Address comments Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- Provide example for dynamic reservation features. Diffs (updated) - src/Makefile.am c479aca src/examples/dynamic_reservation_framework.cpp PRE-CREATION src/tests/dynamic_reservation_framework_test.sh PRE-CREATION src/tests/examples_tests.cpp 3f56b30 src/tests/flags.hpp 984cd4a src/tests/script.cpp d2280c2 Diff: https://reviews.apache.org/r/37168/diff/ Testing --- make make check Thanks, Klaus Ma
Review Request 40375: Support distinguishing revocable resources in the Resource protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40375/ --- Review request for mesos, Guangya Liu, Artem Harutyunyan, and Joris Van Remoortere. Bugs: MESOS-3888 https://issues.apache.org/jira/browse/MESOS-3888 Repository: mesos Description --- MESOS-3888: We need to distinguish revocable resource for usage slack and allocation slack. Diffs - include/mesos/mesos.proto 5ad48bd Diff: https://reviews.apache.org/r/40375/diff/ Testing --- make && make check Thanks, Klaus Ma
Review Request 40379: [WIP] MESOS-3930: Set resource type as USAGE_SLACK for Oversubscription
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40379/ --- Review request for mesos. Bugs: MESOS-3930 https://issues.apache.org/jira/browse/MESOS-3930 Repository: mesos Description --- In Optimistic Offer Phase 1, it introduce `RevocableInfo::type`: USAGE_SLACK for Oversubscription and ALLOCATION_SLACK for Optimistic Offer. Slave helps to update `RevocableInfo::type` for Oversubscription. Diffs - src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee Diff: https://reviews.apache.org/r/40379/diff/ Testing --- make (make check is on-going) Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
gt; > src/tests/flags.hpp, lines 155-158
> > <https://reviews.apache.org/r/37168/diff/5/?file=1072550#file1072550line155>
> >
> > I think we don't need this since we have:
> >
> > ```
> > os::setenv("MESOS_ROLES", flags.role.get());
> > ```
> >
> > This seems to be how other example frameworks such as
> > `persistent_volume_framework.cpp` have gotten away without this explicit
> > `roles` flag.
Agree :). Addressed in new patch.
- Klaus
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review101541
---
On Nov. 9, 2015, 4:10 p.m., Klaus Ma wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
>
> (Updated Nov. 9, 2015, 4:10 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Provide example for dynamic reservation features.
>
>
> Diffs
> -
>
> src/Makefile.am c479aca
> src/examples/dynamic_reservation_framework.cpp PRE-CREATION
> src/tests/dynamic_reservation_framework_test.sh PRE-CREATION
> src/tests/examples_tests.cpp 3f56b30
>
> Diff: https://reviews.apache.org/r/37168/diff/
>
>
> Testing
> ---
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated Nov. 9, 2015, 4:10 p.m.) Review request for mesos and Michael Park. Changes --- Address comments Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- Provide example for dynamic reservation features. Diffs (updated) - src/Makefile.am c479aca src/examples/dynamic_reservation_framework.cpp PRE-CREATION src/tests/dynamic_reservation_framework_test.sh PRE-CREATION src/tests/examples_tests.cpp 3f56b30 Diff: https://reviews.apache.org/r/37168/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.
> On Oct. 26, 2015, 10:07 p.m., Klaus Ma wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1004 > > <https://reviews.apache.org/r/39401/diff/5/?file=1105060#file1105060line1004> > > > > Just check the code, dynamically reserved resource are included in > > allocation. > > Qian Zhang wrote: > Really?:-) I think once a resource is dynamically reserved, it will be > removed from the framework's allocation and marked as reserved in slave's > total resources, so that in next allocation cycle it can be offered to the > framework of the role which reserves it. > > Alexander Rukletsov wrote: > A reserved resource can be allocated or not. For example, if a framework > declines a reserved resource, it will be removed from allocated. Thanks, @AlexR; got your point. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39401/#review104013 --- On Nov. 6, 2015, 2:25 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39401/ > --- > > (Updated Nov. 6, 2015, 2:25 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39401/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39400: Quota: Implemented quota API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39400/#review105575 --- Ship it! Ship It! - Klaus Ma On Nov. 6, 2015, 2:25 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39400/ > --- > > (Updated Nov. 6, 2015, 2:25 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.hpp > cfd937ba306273c24fb5337dfeb1a15e1545169b > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39400/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38335/ --- (Updated Nov. 4, 2015, 9:21 p.m.) Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. Changes --- rebase the code & address comments Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - src/common/http.cpp f56d8a1 src/docker/executor.cpp d4c05c2 src/examples/persistent_volume_framework.cpp 176ac3d src/launcher/executor.cpp 50b3c6e src/master/contender.cpp c641305 src/master/http.cpp 9d20346 src/master/maintenance.cpp 5fe9358 src/master/registrar.cpp 1117232 src/slave/containerizer/fetcher.cpp e0d02d5 src/slave/containerizer/mesos/containerizer.cpp 9fd69c1 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 1911ba6 src/slave/monitor.cpp aa6e958 src/tests/containerizer/launch_tests.cpp de655ec src/tests/containerizer/port_mapping_tests.cpp ae2c0e6 src/tests/fault_tolerance_tests.cpp f78a291 src/tests/master_contender_detector_tests.cpp 1da7f91 src/tests/master_maintenance_tests.cpp e89ce3b src/tests/master_tests.cpp 8564405 src/tests/mesos.cpp ab2d85b src/tests/monitor_tests.cpp 583e711 src/tests/reservation_endpoints_tests.cpp f5f9c48 src/tests/resources_tests.cpp 6584fc6 src/tests/scheduler_http_api_tests.cpp b6f6e91 src/tests/script.cpp bcc1fab src/tests/slave_tests.cpp 91dbdba src/usage/main.cpp 86fd796 Diff: https://reviews.apache.org/r/38335/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/ --- (Updated Nov. 4, 2015, 9:19 p.m.) Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. Changes --- rebase the code Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 Diff: https://reviews.apache.org/r/38342/diff/ Testing --- cd 3rdparty/libprocess/3rdparty/stout ./boostrap ./configure make Thanks, Klaus Ma
Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/#review105425 --- src/master/quota_handler.cpp (line 1) <https://reviews.apache.org/r/36913/#comment164016> It seems other feature named file without "_handler"; any consideration for it? - Klaus Ma On Nov. 3, 2015, 9:55 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36913/ > --- > > (Updated Nov. 3, 2015, 9:55 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, > and Joris Van Remoortere. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Added /quota HTTP Endpoint for Quota handling. > > > Diffs > - > > src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 > src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db > src/master/http.cpp 093f79384916dc08b32b70d3614e0ff314825c42 > src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac > src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea > src/master/quota_handler.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/36913/diff/ > > > Testing > --- > > make check > > > Thanks, > > Joerg Schad > >
Re: Review Request 39399: Quota: Refactored hierarchical allocator in preparation for quota.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39399/#review105427 --- Ship it! Ship It! - Klaus Ma On Nov. 6, 2015, 2:23 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39399/ > --- > > (Updated Nov. 6, 2015, 2:23 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.hpp > cfd937ba306273c24fb5337dfeb1a15e1545169b > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39399/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.
> On Oct. 25, 2015, 8:10 a.m., Klaus Ma wrote: > > include/mesos/master/allocator.hpp, line 358 > > <https://reviews.apache.org/r/38218/diff/7/?file=1105051#file1105051line358> > > > > It said Mesos master will also validate the quota request in design > > document; should we clarify which part is covered by Mesos master and which > > part should be covered by allocator? For example, according to this > > comments, empty role will pass master's checking; it's allocator's > > resposibility to accept or reject it. > > Alexander Rukletsov wrote: > A hard thing here is to specify, what a *custom* allocator should check. > I try to impose as few restrictions as possible. The part of the comment you > refer to says that if an internal invariant (double `setQuota()` call) is > violated. That's OK to me :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38218/#review103910 --- On Nov. 6, 2015, 3:29 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38218/ > --- > > (Updated Nov. 6, 2015, 3:29 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3716 > https://issues.apache.org/jira/browse/MESOS-3716 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 > src/master/allocator/mesos/allocator.hpp > c5375aa89b210e46c41ac7d68d119749de15d2f5 > src/master/allocator/mesos/hierarchical.hpp > cfd937ba306273c24fb5337dfeb1a15e1545169b > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a > > Diff: https://reviews.apache.org/r/38218/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38218/#review105431 --- Ship it! Ship It! - Klaus Ma On Nov. 6, 2015, 3:29 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38218/ > --- > > (Updated Nov. 6, 2015, 3:29 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3716 > https://issues.apache.org/jira/browse/MESOS-3716 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 > src/master/allocator/mesos/allocator.hpp > c5375aa89b210e46c41ac7d68d119749de15d2f5 > src/master/allocator/mesos/hierarchical.hpp > cfd937ba306273c24fb5337dfeb1a15e1545169b > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a > > Diff: https://reviews.apache.org/r/38218/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39285: Added Quota Request Validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39285/#review105440 --- src/master/quota_handler.cpp (line 113) <https://reviews.apache.org/r/39285/#comment164041> Should we also check whether `resource.get().role()` is empty? There should be the case that assign empty string to `resource.role`. src/master/quota_handler.cpp (line 124) <https://reviews.apache.org/r/39285/#comment164043> I'd suggest to move it before checking role to make all simple check together, i.e. `has_disk`, `has_revocable`. src/master/quota_handler.cpp (line 134) <https://reviews.apache.org/r/39285/#comment164044> Suggest to move it into the loop; if any role is not known by master, we did not need to continue to check others. src/master/quota_handler.cpp (line 174) <https://reviews.apache.org/r/39285/#comment164046> Should we move it into `validateQuotaRequest`? If any role is exist in master, we did not need to continue to check others. And in QuotaHandler, we had the pointer to master `Master* master`. - Klaus Ma On Nov. 6, 2015, 11:23 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39285/ > --- > > (Updated Nov. 6, 2015, 11:23 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3199 > https://issues.apache.org/jira/browse/MESOS-3199 > > > Repository: mesos > > > Description > --- > > Added Quota Request Validation. > > > Diffs > - > > src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac > src/master/quota_handler.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39285/diff/ > > > Testing > --- > > > Thanks, > > Joerg Schad > >
Re: Review Request 39317: Quota: Moved QuotaInfo protobuf into a separate package.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39317/#review105420 --- Ship it! Ship It! - Klaus Ma On Nov. 6, 2015, 3:28 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39317/ > --- > > (Updated Nov. 6, 2015, 3:28 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3164 > https://issues.apache.org/jira/browse/MESOS-3164 > > > Repository: mesos > > > Description > --- > > Quota: Moved QuotaInfo protobuf into a separate package. > > > Diffs > - > > include/mesos/master/quota.hpp 5f7822f40af6fb23cdafdd0c205bcdc67e596935 > include/mesos/master/quota.proto d2e3a45735e4ebcf257682556aff5075e6e3bf79 > src/CMakeLists.txt e6169a0e3ad34dd0e4c3430a6532bd48c4bd04fd > src/Makefile.am 98cbafc134ec388a176d50172912fbfdf9f5bfa3 > > Diff: https://reviews.apache.org/r/39317/diff/ > > > Testing > --- > > make check > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField
> On Oct. 23, 2015, 4:53 a.m., Jan Schlicht wrote: > > src/slave/containerizer/mesos/containerizer.cpp, line 800 > > <https://reviews.apache.org/r/38335/diff/4/?file=1101469#file1101469line800> > > > > Not yours, but please s/push_back/emplace_back > > Michael Park wrote: > I'm curious as to what your reasoning is? AFAIK, that'll avoid temp var copy from `JSON::protobuf(...)`'s return to `commandArray.values` - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38335/#review103655 ------- On Oct. 20, 2015, 1:37 p.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38335/ > --- > > (Updated Oct. 20, 2015, 1:37 p.m.) > > > Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. > > > Bugs: MESOS-3405 > https://issues.apache.org/jira/browse/MESOS-3405 > > > Repository: mesos > > > Description > --- > > Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which > converts a `google::protobuf::Message` into a `JSON::Object`. > We should add the support for `google::protobuf::RepeatedPtrField` by > introducing overloaded functions. > > > Diffs > - > > src/common/http.cpp f56d8a1 > src/docker/executor.cpp 1e49013 > src/examples/persistent_volume_framework.cpp 176ac3d > src/launcher/executor.cpp 50b3c6e > src/master/contender.cpp c641305 > src/master/http.cpp 093f793 > src/master/maintenance.cpp 5fe9358 > src/master/registrar.cpp 1117232 > src/slave/containerizer/fetcher.cpp e0d02d5 > src/slave/containerizer/mesos/containerizer.cpp d1fc5a4 > src/slave/monitor.cpp aa6e958 > src/tests/containerizer/launch_tests.cpp de655ec > src/tests/fault_tolerance_tests.cpp f78a291 > src/tests/master_maintenance_tests.cpp e89ce3b > src/tests/master_tests.cpp ee24739 > src/tests/mesos.hpp 3e58b45 > src/tests/mesos.cpp ab2d85b > src/tests/monitor_tests.cpp 583e711 > src/tests/reservation_endpoints_tests.cpp f5f9c48 > src/tests/resources_tests.cpp 6584fc6 > src/tests/scheduler_http_api_tests.cpp d338a1b > src/tests/script.cpp bcc1fab > src/tests/slave_tests.cpp 10a4fa7 > src/usage/main.cpp 86fd796 > > Diff: https://reviews.apache.org/r/38335/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38335/ --- (Updated Nov. 3, 2015, 9:38 p.m.) Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. Changes --- Address comments Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - src/common/http.cpp f56d8a1 src/docker/executor.cpp d4c05c2 src/examples/persistent_volume_framework.cpp 176ac3d src/launcher/executor.cpp 50b3c6e src/master/contender.cpp c641305 src/master/http.cpp 093f793 src/master/maintenance.cpp 5fe9358 src/master/registrar.cpp 1117232 src/slave/containerizer/fetcher.cpp e0d02d5 src/slave/containerizer/mesos/containerizer.cpp 9fd69c1 src/slave/monitor.cpp aa6e958 src/tests/containerizer/launch_tests.cpp de655ec src/tests/fault_tolerance_tests.cpp f78a291 src/tests/master_maintenance_tests.cpp e89ce3b src/tests/master_tests.cpp ee24739 src/tests/mesos.cpp ab2d85b src/tests/monitor_tests.cpp 583e711 src/tests/reservation_endpoints_tests.cpp f5f9c48 src/tests/resources_tests.cpp 6584fc6 src/tests/scheduler_http_api_tests.cpp b6f6e91 src/tests/script.cpp bcc1fab src/tests/slave_tests.cpp 91dbdba src/usage/main.cpp 86fd796 Diff: https://reviews.apache.org/r/38335/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
> On Oct. 18, 2015, 11:02 p.m., Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 764-768 > > <https://reviews.apache.org/r/38342/diff/6/?file=1099827#file1099827line764> > > > > As mentioned before, let's remove the `default` case entirely. Refer to > > [MESOS-3754](https://issues.apache.org/jira/browse/MESOS-3754) for details. I just remove `default:` becase keep `ABORT` for deprecated case (`google::protobuf::FieldDescriptor::TYPE_GROUP:`). > On Oct. 18, 2015, 11:02 p.m., Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, lines 182-189 > > <https://reviews.apache.org/r/38342/diff/6/?file=1099828#file1099828line182> > > > > Indent 2 spaces, not the strings themselves, but within the strings. The 2 sparces indent are in the strings, do you mean we should move it out? - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/#review103078 ------- On Oct. 16, 2015, 9:56 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38342/ > --- > > (Updated Oct. 16, 2015, 9:56 a.m.) > > > Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. > > > Bugs: MESOS-3405 > https://issues.apache.org/jira/browse/MESOS-3405 > > > Repository: mesos > > > Description > --- > > Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which > converts a `google::protobuf::Message` into a `JSON::Object`. > We should add the support for `google::protobuf::RepeatedPtrField` by > introducing overloaded functions. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 > > Diff: https://reviews.apache.org/r/38342/diff/ > > > Testing > --- > > cd 3rdparty/libprocess/3rdparty/stout > ./boostrap > ./configure > make > > > Thanks, > > Klaus Ma > >
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/ --- (Updated Oct. 16, 2015, 9:56 a.m.) Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. Changes --- Addressed comments. Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 Diff: https://reviews.apache.org/r/38342/diff/ Testing --- cd 3rdparty/libprocess/3rdparty/stout ./boostrap ./configure make Thanks, Klaus Ma
Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38335/ --- (Updated Oct. 20, 2015, 5:37 a.m.) Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. Changes --- Address comments Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - src/common/http.cpp f56d8a1 src/docker/executor.cpp 1e49013 src/examples/persistent_volume_framework.cpp 176ac3d src/launcher/executor.cpp 50b3c6e src/master/contender.cpp c641305 src/master/http.cpp 093f793 src/master/maintenance.cpp 5fe9358 src/master/registrar.cpp 1117232 src/slave/containerizer/fetcher.cpp e0d02d5 src/slave/containerizer/mesos/containerizer.cpp d1fc5a4 src/slave/monitor.cpp aa6e958 src/tests/containerizer/launch_tests.cpp de655ec src/tests/fault_tolerance_tests.cpp f78a291 src/tests/master_maintenance_tests.cpp e89ce3b src/tests/master_tests.cpp ee24739 src/tests/mesos.hpp 3e58b45 src/tests/mesos.cpp ab2d85b src/tests/monitor_tests.cpp 583e711 src/tests/reservation_endpoints_tests.cpp f5f9c48 src/tests/resources_tests.cpp 6584fc6 src/tests/scheduler_http_api_tests.cpp d338a1b src/tests/script.cpp bcc1fab src/tests/slave_tests.cpp 10a4fa7 src/usage/main.cpp 86fd796 Diff: https://reviews.apache.org/r/38335/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/#review103214 --- The code diff of #38335 are alos upedted based on master. - Klaus Ma On Oct. 20, 2015, 2:39 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38342/ > --- > > (Updated Oct. 20, 2015, 2:39 a.m.) > > > Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. > > > Bugs: MESOS-3405 > https://issues.apache.org/jira/browse/MESOS-3405 > > > Repository: mesos > > > Description > --- > > Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which > converts a `google::protobuf::Message` into a `JSON::Object`. > We should add the support for `google::protobuf::RepeatedPtrField` by > introducing overloaded functions. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 > > Diff: https://reviews.apache.org/r/38342/diff/ > > > Testing > --- > > cd 3rdparty/libprocess/3rdparty/stout > ./boostrap > ./configure > make > > > Thanks, > > Klaus Ma > >
Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField
> On Sept. 29, 2015, 3:48 p.m., Alexander Rukletsov wrote: > > src/tests/mesos.hpp, line 1844 > > <https://reviews.apache.org/r/38335/diff/3/?file=1084054#file1084054line1844> > > > > We tend not to use `typedef`s in the codebase. However, this looks like > > a good idea, but I would place it in `Resources`, e.g. > > `Resources::ProtoType`. > > > > However, I'll leave the decision whether to use `typedef` or not to you > > and your shepherd. It's related to test only, so keep it in test module for now. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38335/#review100961 ------- On Oct. 20, 2015, 5:37 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38335/ > --- > > (Updated Oct. 20, 2015, 5:37 a.m.) > > > Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. > > > Bugs: MESOS-3405 > https://issues.apache.org/jira/browse/MESOS-3405 > > > Repository: mesos > > > Description > --- > > Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which > converts a `google::protobuf::Message` into a `JSON::Object`. > We should add the support for `google::protobuf::RepeatedPtrField` by > introducing overloaded functions. > > > Diffs > - > > src/common/http.cpp f56d8a1 > src/docker/executor.cpp 1e49013 > src/examples/persistent_volume_framework.cpp 176ac3d > src/launcher/executor.cpp 50b3c6e > src/master/contender.cpp c641305 > src/master/http.cpp 093f793 > src/master/maintenance.cpp 5fe9358 > src/master/registrar.cpp 1117232 > src/slave/containerizer/fetcher.cpp e0d02d5 > src/slave/containerizer/mesos/containerizer.cpp d1fc5a4 > src/slave/monitor.cpp aa6e958 > src/tests/containerizer/launch_tests.cpp de655ec > src/tests/fault_tolerance_tests.cpp f78a291 > src/tests/master_maintenance_tests.cpp e89ce3b > src/tests/master_tests.cpp ee24739 > src/tests/mesos.hpp 3e58b45 > src/tests/mesos.cpp ab2d85b > src/tests/monitor_tests.cpp 583e711 > src/tests/reservation_endpoints_tests.cpp f5f9c48 > src/tests/resources_tests.cpp 6584fc6 > src/tests/scheduler_http_api_tests.cpp d338a1b > src/tests/script.cpp bcc1fab > src/tests/slave_tests.cpp 10a4fa7 > src/usage/main.cpp 86fd796 > > Diff: https://reviews.apache.org/r/38335/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/ --- (Updated Oct. 20, 2015, 2:39 a.m.) Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht. Changes --- Address comments Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 Diff: https://reviews.apache.org/r/38342/diff/ Testing --- cd 3rdparty/libprocess/3rdparty/stout ./boostrap ./configure make Thanks, Klaus Ma
Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38627/#review103365 --- src/module/manager.hpp (line 94) <https://reviews.apache.org/r/38627/#comment161383> Should we merge with `moduleParameters[moduleName]` or replace it? IMO, prefer to merge them; I'd like to leave it to you and your shepherd. src/module/manager.hpp (line 104) <https://reviews.apache.org/r/38627/#comment161384> Prefer to add a `Option` parameter to create instead of a new function. Such as static Try<T*> create(const std::string& moduleName, const Option& params) - Klaus Ma On Oct. 20, 2015, 9:52 p.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38627/ > --- > > (Updated Oct. 20, 2015, 9:52 p.m.) > > > Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till > Toenshoff. > > > Bugs: MESOS-3072 > https://issues.apache.org/jira/browse/MESOS-3072 > > > Repository: mesos > > > Description > --- > > Allows developers to provide their own parameters when loading modules > instead of using the ones provided by the user when loading Mesos. This helps > to deal with default modules (those used when the user doesn't provide any), > and for testing of the modules. > > > Diffs > - > > src/examples/example_module_impl.cpp > db015cea130701a4e0a6fcb890c79fbb0c02c1ce > src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 > src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef > src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 > src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 > > Diff: https://reviews.apache.org/r/38627/diff/ > > > Testing > --- > > make check > > > Thanks, > > Alexander Rojas > >
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> >
> > After looking at our http code, I realized that we use the same pattern
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> > array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way
> > for rendering JSON from protobuf is used. Without digging deep inside, I
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> >
> > The proposed conversion function covers one of the possible ways. How about
> > add one more convertion? Something like:
> > ```
> > template
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> > static_assert(std::is_convertible<T*, google::protobuf::Message*>::value,
> > "T must be a google::protobuf::Message");
> > JSON::Array array;
> > array.values.reserve(repeated.size());
> > foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> > }
> >
> > return array;
> > }
> > ```
> >
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] =
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo&
> > info) { return model(info); });
> > ```
> >
> > A further improvement would be to accept any iterable collection, not only
> > `RepeatedPtrField<>`, for example `hashset`.
> >
> > What do you think?
>
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function`
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in
> our code :).
>
> I'll also address your comments above.
>
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229
>
> Michael Park wrote:
> I like the idea of taking a projection function as an argument, but let's
> do it as a separate ticket to keep the scope of this review narrow.
>
> Klaus Ma wrote:
> I'll log a new ticket to trace projection fuction part.
>
> Klaus Ma wrote:
> MESOS-3580 is logged to trace this requirement, @alex-mesos, would you
> shepherd it?
>
> Alexander Rukletsov wrote:
> Great, thanks! I'm not a Mesos committer, hence I cannot shepherd, but I
> would love to review and help out with that! Maybe @MPark will agree to
> shepherd?
>
> Michael Park wrote:
> @klaus1982: Thanks for filing MESOS-3580. I think we'll probably hold off
> working on that ticket to see if it'll still be useful after the work around
> speeding up JSON parsing/streaming.
OK, got it for MESOS-3580.
- Klaus
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38342/#review100964
---
On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
>
> (Updated Oct. 4, 2015, 11:29 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
>
>
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by
> introducing overloaded functions.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9
>
> Diff: https://reviews.apache.org/r/38342/diff/
>
>
> Testing
> ---
>
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 39056: Fix for Mesos master crash due to check failure.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39056/#review101727 --- src/common/resources.cpp (line 875) <https://reviews.apache.org/r/39056/#comment159198> This fix is ok for this ticket; but how to handle other part about cpu()? Here's some question from me: 1. if `epsilon` is 0.01 here, does it mean the min cpu is 0.01? 2. is this the only code that need `epsilon`? It seems not @jieyu/@mcypark, show we start a EPIC to include all precision related ticket? so, we can 1. unify the min cpu/men/disk to user 2. unify the operator/code within allocator 3. unify the precision between backend and UI 4. clarify the requirement to cpu/mem, e.g. whether accept empty cpu/mem Any comments? - Klaus Ma On Oct. 6, 2015, 10:07 p.m., Mandeep Chadha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39056/ > --- > > (Updated Oct. 6, 2015, 10:07 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-3552 > https://issues.apache.org/jira/browse/MESOS-3552 > > > Repository: mesos > > > Description > --- > > Check failed due to double comparison : MESOS-3552. > > > Diffs > - > > src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 > src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e > src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 > > Diff: https://reviews.apache.org/r/39056/diff/ > > > Testing > --- > > Added unit test. > make check successful. > > > Thanks, > > Mandeep Chadha > >
Re: Review Request 39154: Log IP addresses together with failure messages.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39154/#review102002
---
3rdparty/libprocess/src/poll_socket.cpp (line 31)
<https://reviews.apache.org/r/39154/#comment159523>
Would help me to understand why `anonymous` is necessary here?
3rdparty/libprocess/src/poll_socket.cpp (line 32)
<https://reviews.apache.org/r/39154/#comment159525>
Personally, `stringify_` maybe better.
3rdparty/libprocess/src/poll_socket.cpp (line 113)
<https://reviews.apache.org/r/39154/#comment159528>
`Socket` has the following API, is it OK to use to get address?
Try address() const
{
return impl->address();
}
3rdparty/libprocess/src/poll_socket.cpp (line 167)
<https://reviews.apache.org/r/39154/#comment159524>
s/.//
3rdparty/libprocess/src/poll_socket.cpp (line 178)
<https://reviews.apache.org/r/39154/#comment159527>
s/failed/ failed/
3rdparty/libprocess/src/poll_socket.cpp (line 206)
<https://reviews.apache.org/r/39154/#comment159526>
s/failed/ failed/
3rdparty/libprocess/src/poll_socket.cpp (line 218)
<https://reviews.apache.org/r/39154/#comment159529>
Can we pass `this` as parameter? So all info are there in
`socket_send_data`; I'd like to leave this to your shepherd.
- Klaus Ma
On Oct. 9, 2015, 1:42 a.m., Brice Arnould wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39154/
> ---
>
> (Updated Oct. 9, 2015, 1:42 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1661
> https://issues.apache.org/jira/browse/MESOS-1661
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Log IP addresses together with failure messages.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/src/poll_socket.cpp
> 28ed102972a9d8f88048aea4046ed837b6a25b35
>
> Diff: https://reviews.apache.org/r/39154/diff/
>
>
> Testing
> ---
>
> Ran "make check"
>
>
> Thanks,
>
> Brice Arnould
>
>
Re: Review Request 39154: Log IP addresses together with failure messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39154/#review102003 --- BTW, please add your shepherd into pepole field. - Klaus Ma On Oct. 9, 2015, 1:42 a.m., Brice Arnould wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39154/ > --- > > (Updated Oct. 9, 2015, 1:42 a.m.) > > > Review request for mesos. > > > Bugs: MESOS-1661 > https://issues.apache.org/jira/browse/MESOS-1661 > > > Repository: mesos > > > Description > --- > > Log IP addresses together with failure messages. > > > Diffs > - > > 3rdparty/libprocess/src/poll_socket.cpp > 28ed102972a9d8f88048aea4046ed837b6a25b35 > > Diff: https://reviews.apache.org/r/39154/diff/ > > > Testing > --- > > Ran "make check" > > > Thanks, > > Brice Arnould > >
Re: Review Request 40375: Support distinguishing revocable resources in the Resource protobuf.
> On Nov. 18, 2015, 2:11 a.m., Vinod Kone wrote: > > include/mesos/mesos.proto, line 651 > > <https://reviews.apache.org/r/40375/diff/1/?file=1127795#file1127795line651> > > > > I don't think adding a required field to a protobuf that is sent across > > the wire is backwards compatible. This should be optional with default as > > USAGE_SLACK. Agree; we should keep backwards compatible. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40375/#review106898 ------- On Nov. 17, 2015, 11:30 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40375/ > --- > > (Updated Nov. 17, 2015, 11:30 a.m.) > > > Review request for mesos, Guangya Liu, Artem Harutyunyan, and Joris Van > Remoortere. > > > Bugs: MESOS-3888 > https://issues.apache.org/jira/browse/MESOS-3888 > > > Repository: mesos > > > Description > --- > > MESOS-3888: We need to distinguish revocable resource for usage slack and > allocation slack. > > > Diffs > - > > include/mesos/mesos.proto 5ad48bd > > Diff: https://reviews.apache.org/r/40375/diff/ > > > Testing > --- > > make && make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 37531: MESOS-3070
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/ --- (Updated Aug. 28, 2015, 9:48 a.m.) Review request for mesos. Changes --- Add UT case Bugs: MESOS-3070 https://issues.apache.org/jira/browse/MESOS-3070 Repository: mesos Description --- MESOS-3070 (Master CHECK failure if a framework uses duplicated task id.) Diffs (updated) - src/master/http.cpp 37d76ee src/master/master.hpp 36c6759 src/master/master.cpp 95207d2 src/tests/master_tests.cpp 8a6b98b Diff: https://reviews.apache.org/r/37531/diff/ Testing (updated) --- make make check Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063
The tests failed because script.cpp can not find test script which has been updated; I'm working to check it, although the root cause is un-clear now. [ RUN ] ExamplesTest.DynamicReservationFramework ../../src/tests/script.cpp:66: Failure Failed Failed to locate script: No such file or directory [ FAILED ] ExamplesTest.DynamicReservationFramework (0 ms) On 2015年08月27日 12:19, Mesos ReviewBot wrote: [ RUN ] ExamplesTest.DynamicReservationFramework ../../src/tests/script.cpp:66: Failure Failed Failed to locate script: No such file or directory [ FAILED ] ExamplesTest.DynamicReservationFramework (0 ms) -- Klaus Ma (马达) | PMP | kl...@cguru.net
Re: Review Request 37168: MESOS-3063
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated 八月 27, 2015, 3:33 a.m.) Review request for mesos. Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- MESOS-3063 (Add an example framework using dynamic reservation) Diffs (updated) - src/Makefile.am 7b620ff src/examples/dynamic_reservation_executor.cpp PRE-CREATION src/examples/dynamic_reservation_framework.cpp PRE-CREATION src/tests/dynamic_reservation_framework_test.sh PRE-CREATION src/tests/examples_tests.cpp 3f56b30 src/tests/flags.hpp 3644956 src/tests/script.cpp bcc1fab Diff: https://reviews.apache.org/r/37168/diff/ Testing (updated) --- make make check Thanks, Klaus Ma
Re: Review Request 38102: MESOS-3046
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38102/ --- (Updated Sept. 3, 2015, 2:01 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3046 https://issues.apache.org/jira/browse/MESOS-3046 Repository: mesos Description --- MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random) Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp e90dabb0c572923a50490ecb17867dc50c6d161d Diff: https://reviews.apache.org/r/38102/diff/ Testing --- make make check Thanks, Klaus Ma
Review Request 38102: MESOS-3046
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38102/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random) Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp e90dabb0c572923a50490ecb17867dc50c6d161d Diff: https://reviews.apache.org/r/38102/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 37531: MESOS-3070 (Master CHECK failure if a framework uses duplicated task id)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/ --- (Updated Sept. 5, 2015, 3:27 a.m.) Review request for mesos and Vinod Kone. Changes --- Add summary & description Summary (updated) - MESOS-3070 (Master CHECK failure if a framework uses duplicated task id) Bugs: MESOS-3070 https://issues.apache.org/jira/browse/MESOS-3070 Repository: mesos Description (updated) --- __Phenomenon:__ The master crash because of duplicated task id __Root Cause:__ The task id are stored in slave agent; if master failover, there's a time window that new slave lanched a task with same task id; so if the old task re-registered back, the master will crash because of duplicated task id. __Solution:__ Stores tasks info in Master::Framework by SlaveID to avoid duplicated issue. Diffs - src/master/http.cpp 37d76ee src/master/master.hpp 36c6759 src/master/master.cpp 95207d2 src/tests/master_tests.cpp 8a6b98b Diff: https://reviews.apache.org/r/37531/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated Sept. 5, 2015, 3:14 a.m.) Review request for mesos and Michael Park. Changes --- Update summary & description Summary (updated) - MESOS-3063 (Add an example framework using dynamic reservation) Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description (updated) --- Provide example for dynamic reservation features. Diffs - src/Makefile.am 7b620ff src/examples/dynamic_reservation_executor.cpp PRE-CREATION src/examples/dynamic_reservation_framework.cpp PRE-CREATION src/tests/dynamic_reservation_framework_test.sh PRE-CREATION src/tests/examples_tests.cpp 3f56b30 src/tests/flags.hpp 3644956 src/tests/script.cpp bcc1fab Diff: https://reviews.apache.org/r/37168/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated Sept. 6, 2015, 4:11 a.m.) Review request for mesos and Michael Park. Changes --- Update the code diff to address commnets: * Add comments to describe the example logic * Use command executor instead of test_executor.cpp Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- Provide example for dynamic reservation features. Diffs (updated) - src/Makefile.am 5fdca0f src/examples/dynamic_reservation_framework.cpp PRE-CREATION src/tests/dynamic_reservation_framework_test.sh PRE-CREATION src/tests/examples_tests.cpp 3f56b30 src/tests/flags.hpp 06da36d src/tests/script.cpp bcc1fab Diff: https://reviews.apache.org/r/37168/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38165/#review97950 --- src/docker/docker.cpp (line 414) <https://reviews.apache.org/r/38165/#comment154086> Should we provide UT for this fix? please refer to the other test in "src/tests". - Klaus Ma On Sept. 7, 2015, 6:47 p.m., Wojciech Sielski wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38165/ > --- > > (Updated Sept. 7, 2015, 6:47 p.m.) > > > Review request for mesos. > > > Bugs: MESOS-3377 > https://issues.apache.org/jira/browse/MESOS-3377 > > > Repository: mesos > > > Description > --- > > MESOS-3377 - Adding CONTAINER_NAME as additional env variable > > > Diffs > - > > src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e > > Diff: https://reviews.apache.org/r/38165/diff/ > > > Testing > --- > > Every docker container that is created by mesos, > gonna get conatiner dynamic env variable CONTAINER_NAME. > So it is easy to use by 3rd party application, > for example: when use with registartor/consul > so is it's much easier to deregister itself > (or put into maintanance mode) before being killed. > > > Tested and verified afetr build locally. > > > Thanks, > > Wojciech Sielski > >
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
Hi Michael, Do you have more comments about this example? On 2015年09月06日 21:28, Klaus Ma wrote: This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote: src/Makefile.am <https://reviews.apache.org/r/37168/diff/4/?file=1064719#file1064719line1463> (Diff revision 4) 1463 Any reason for this blank line here? It's not necessary, will delete it. On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote: src/examples/dynamic_reservation_framework.cpp <https://reviews.apache.org/r/37168/diff/4/?file=1064720#file1064720line281> (Diff revision 4) 281 START, // The framework get the offer for the first time. s/get/receives OK :). On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote: src/examples/dynamic_reservation_framework.cpp <https://reviews.apache.org/r/37168/diff/4/?file=1064720#file1064720line283> (Diff revision 4) 283 TASK_DONE, // All tasks are done. s/TASK_DONE/TASKS_DONE Please correct me if I am wrong: TASK_DONE is a global state across all slaves, while the other states can differ per state, or? Maybe just extend the comment a bit explaining this. Yes, TASK_DONE is a global state; when all task finished, framework will un-reserve all resources. On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote: src/tests/dynamic_reservation_framework_test.sh <https://reviews.apache.org/r/37168/diff/4/?file=1064721#file1064721line30> (Diff revision 4) 30 export MESOS_ROLES="test" Do we need this here and below in script.cpp? Yes, it's necessary because: 1. dynamic reservation is role based 2. can not use default role for dynamic reservation, neither * nor MESOS_DEFAULT_ROLE - Klaus On September 6th, 2015, 4:11 a.m. UTC, Klaus Ma wrote: Review request for mesos and Michael Park. By Klaus Ma. /Updated Sept. 6, 2015, 4:11 a.m./ *Bugs: * MESOS-3063 <https://issues.apache.org/jira/browse/MESOS-3063> *Repository: * mesos Description Provide example for dynamic reservation features. Testing make make check Diffs * src/Makefile.am (5fdca0f) * src/examples/dynamic_reservation_framework.cpp (PRE-CREATION) * src/tests/dynamic_reservation_framework_test.sh (PRE-CREATION) * src/tests/examples_tests.cpp (3f56b30) * src/tests/flags.hpp (06da36d) * src/tests/script.cpp (bcc1fab) View Diff <https://reviews.apache.org/r/37168/diff/> -- Klaus Ma (马达), PMP® | http://www.cguru.net
Review Request 38201: [MESOS-1187] precision errors with allocation calculations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38201/ --- Review request for mesos. Bugs: MESOS-1187 https://issues.apache.org/jira/browse/MESOS-1187 Repository: mesos Description --- As allocations are stored/transmitted as doubles many a times precision errors creep in. Diffs - src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 Diff: https://reviews.apache.org/r/38201/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38003: MESOS-3351
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38003/ --- (Updated Sept. 1, 2015, 2:44 p.m.) Review request for mesos and Vinod Kone. Changes --- Add JIRA link Bugs: MESOS-3351 https://issues.apache.org/jira/browse/MESOS-3351 Repository: mesos Description --- MESOS-3351 (duplicated slave id in master after master failover) Diffs - src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 Diff: https://reviews.apache.org/r/38003/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 37622: Maintenance Primitives: Shutdown & remove slave when maintenance is started.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37622/#review97884 --- src/master/http.cpp (line 1570) <https://reviews.apache.org/r/37622/#comment153950> Should be LostSlaveMessage? - Klaus Ma On Sept. 2, 2015, 7:33 p.m., Joris Van Remoortere wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37622/ > --- > > (Updated Sept. 2, 2015, 7:33 p.m.) > > > Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu. > > > Bugs: MESOS-1474 > https://issues.apache.org/jira/browse/MESOS-1474 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/http.cpp 94e97a2898106579434e8cdec04b7b0e130a810e > src/tests/master_maintenance_tests.cpp > fb8dca3757a9565d5eb5a69eed10aa34602bb15c > > Diff: https://reviews.apache.org/r/37622/diff/ > > > Testing > --- > > > Thanks, > > Joris Van Remoortere > >
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
> On Sept. 6, 2015, 8:49 a.m., Joerg Schad wrote: > > src/examples/dynamic_reservation_framework.cpp, line 283 > > <https://reviews.apache.org/r/37168/diff/4/?file=1064720#file1064720line283> > > > > s/TASK_DONE/TASKS_DONE > > > > Please correct me if I am wrong: TASK_DONE is a global state across all > > slaves, while the other states can differ per state, or? Maybe just extend > > the comment a bit explaining this. Yes, TASK_DONE is a global state; when all task finished, framework will un-reserve all resources. > On Sept. 6, 2015, 8:49 a.m., Joerg Schad wrote: > > src/tests/dynamic_reservation_framework_test.sh, line 30 > > <https://reviews.apache.org/r/37168/diff/4/?file=1064721#file1064721line30> > > > > Do we need this here and below in script.cpp? Yes, it's necessary because: 1. dynamic reservation is role based 2. can not use default role for dynamic reservation, neither * nor MESOS_DEFAULT_ROLE > On Sept. 6, 2015, 8:49 a.m., Joerg Schad wrote: > > src/Makefile.am, line 1463 > > <https://reviews.apache.org/r/37168/diff/4/?file=1064719#file1064719line1463> > > > > Any reason for this blank line here? It's not necessary, will delete it. > On Sept. 6, 2015, 8:49 a.m., Joerg Schad wrote: > > src/examples/dynamic_reservation_framework.cpp, line 281 > > <https://reviews.apache.org/r/37168/diff/4/?file=1064720#file1064720line281> > > > > s/get/receives OK :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review97885 --- On Sept. 6, 2015, 4:11 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37168/ > --- > > (Updated Sept. 6, 2015, 4:11 a.m.) > > > Review request for mesos and Michael Park. > > > Bugs: MESOS-3063 > https://issues.apache.org/jira/browse/MESOS-3063 > > > Repository: mesos > > > Description > --- > > Provide example for dynamic reservation features. > > > Diffs > - > > src/Makefile.am 5fdca0f > src/examples/dynamic_reservation_framework.cpp PRE-CREATION > src/tests/dynamic_reservation_framework_test.sh PRE-CREATION > src/tests/examples_tests.cpp 3f56b30 > src/tests/flags.hpp 06da36d > src/tests/script.cpp bcc1fab > > Diff: https://reviews.apache.org/r/37168/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38003: MESOS-3351 (duplicated slave id in master after master failover)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38003/ --- (Updated Sept. 5, 2015, 2:46 a.m.) Review request for mesos and Vinod Kone. Changes --- Address Vinod's comments Summary (updated) - MESOS-3351 (duplicated slave id in master after master failover) Bugs: MESOS-3351 https://issues.apache.org/jira/browse/MESOS-3351 Repository: mesos Description (updated) --- __Phenomenon:__ In some race condition, the slave was shutdown when after master failover. __Root Cause:__ The slave was shutdown because of duplicated SlavID: in master, the SlaveID is genereated by masterInfo.id + "-S" + nextSlaveId; when master failover, nextSlaveId was reset to 0 and masterInfo.id (generated by date + ip + port + pid) maybe un-changed which lead to duplicated SlaveID. __Solution/Fix:__ Generate masterInfo.id by UUID instead of "date + ip + port + pid". Diffs (updated) - src/master/master.cpp 5589eca src/tests/master_tests.cpp 8a6b98b Diff: https://reviews.apache.org/r/38003/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38003: MESOS-3351 (duplicated slave id in master after master failover)
> On Sept. 4, 2015, 7:35 p.m., Vinod Kone wrote: > > > > Vinod Kone wrote: > Also, please make the summary and description more meaningful than just > the ticket ID. Yes, both summary & description are updated for this fix > On Sept. 4, 2015, 7:35 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 306-317 > > <https://reviews.apache.org/r/38003/diff/1/?file=1061118#file1061118line306> > > > > Just do this. > > > > ``` > > > > // Master ID is generated randomly based on UUID. > > info_.set_id(UUID::random().toString()); > > > > ``` addressed > On Sept. 4, 2015, 7:35 p.m., Vinod Kone wrote: > > src/tests/master_tests.cpp, line 3607 > > <https://reviews.apache.org/r/38003/diff/1/?file=1061119#file1061119line3607> > > > > All our comments are expected to be proper sentences, i.e., start with > > a capital letter and end with period. Please fix here and everywhere. addressed > On Sept. 4, 2015, 7:35 p.m., Vinod Kone wrote: > > src/tests/master_tests.cpp, lines 3638-3666 > > <https://reviews.apache.org/r/38003/diff/1/?file=1061119#file1061119line3638> > > > > Why do you need to launch a scheduler and task for this test? > > > > I think you can simplify this test by not launching them. Agree, scheduler & tasks are not necessary, both of them are removed. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38003/#review97785 --- On Sept. 5, 2015, 2:46 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38003/ > --- > > (Updated Sept. 5, 2015, 2:46 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-3351 > https://issues.apache.org/jira/browse/MESOS-3351 > > > Repository: mesos > > > Description > --- > > __Phenomenon:__ > In some race condition, the slave was shutdown when after master failover. > > __Root Cause:__ > The slave was shutdown because of duplicated SlavID: in master, the SlaveID > is genereated by masterInfo.id + "-S" + nextSlaveId; when master failover, > nextSlaveId was reset to 0 and masterInfo.id (generated by date + ip + port + > pid) maybe un-changed which lead to duplicated SlaveID. > > __Solution/Fix:__ > Generate masterInfo.id by UUID instead of "date + ip + port + pid". > > > Diffs > - > > src/master/master.cpp 5589eca > src/tests/master_tests.cpp 8a6b98b > > Diff: https://reviews.apache.org/r/38003/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38102: MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38102/ --- (Updated Sept. 5, 2015, 3:08 a.m.) Review request for mesos and Ben Mahler. Changes --- Update summary & description Summary (updated) - MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random) Bugs: MESOS-3046 https://issues.apache.org/jira/browse/MESOS-3046 Repository: mesos Description (updated) --- __Phenomenon:__ Performance downgrade __Root Cause:__ stout's UUID abstraction is re-seeding the random generator during each call to UUID::random(), which is really expensive. __Solution:__ Seeding the random generator only once per thread. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp e90dabb0c572923a50490ecb17867dc50c6d161d Diff: https://reviews.apache.org/r/38102/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063
> On Sept. 2, 2015, 7:30 a.m., Joerg Schad wrote: > > As this is an example framework (which will be used by people to start > > their own framework), would it make sense to add some more comments > > explaining especially the relevant bits here? Agree, let me add more comments for the detail :); that'll be helpful to our uers. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review97423 --- On Aug. 28, 2015, 3:31 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37168/ > --- > > (Updated Aug. 28, 2015, 3:31 a.m.) > > > Review request for mesos and Michael Park. > > > Bugs: MESOS-3063 > https://issues.apache.org/jira/browse/MESOS-3063 > > > Repository: mesos > > > Description > --- > > MESOS-3063 (Add an example framework using dynamic reservation) > > > Diffs > - > > src/Makefile.am 7b620ff > src/examples/dynamic_reservation_executor.cpp PRE-CREATION > src/examples/dynamic_reservation_framework.cpp PRE-CREATION > src/tests/dynamic_reservation_framework_test.sh PRE-CREATION > src/tests/examples_tests.cpp 3f56b30 > src/tests/flags.hpp 3644956 > src/tests/script.cpp bcc1fab > > Diff: https://reviews.apache.org/r/37168/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38050/#review97434 --- src/master/master.cpp <https://reviews.apache.org/r/38050/#comment153311> The indent seems incorrect, here's the suggestion: install( ::pong); src/slave/slave.cpp <https://reviews.apache.org/r/38050/#comment153312> In slave's UT case, here're at least two cases about PING/PONG package: PingTimeoutSomePings & PingTimeoutNoPings; are those UT cases impacted? If so please help to update accordingly; and please also help to check the other UT cases about PING/PONG message, I'd not go through all slave UT cases. - Klaus Ma On Sept. 2, 2015, 12:11 p.m., Yong Qiao Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38050/ > --- > > (Updated Sept. 2, 2015, 12:11 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-1831 and MESOS-1832 > https://issues.apache.org/jira/browse/MESOS-1831 > https://issues.apache.org/jira/browse/MESOS-1832 > > > Repository: mesos > > > Description > --- > > Master should send PingSlaveMessage instead of "PING"; Slave should accept > PingSlaveMessage but not "PING" message; > > > Diffs > - > > src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 > src/slave/slave.hpp 09172f7ed547049b3bd169b3db9be94e14f6bc39 > src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 > > Diff: https://reviews.apache.org/r/38050/diff/ > > > Testing > --- > > > Thanks, > > Yong Qiao Wang > >
Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/ --- Review request for mesos and Bernd Mathiske. Bugs: MESOS-3340 https://issues.apache.org/jira/browse/MESOS-3340 Repository: mesos Description --- Currently, it appears that re-defining a flag on the command-line that was already defined via a OS Env var (MESOS_*) causes the Master to fail with a not very helpful message. For example, if one has MESOS_QUORUM defined, this happens: $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 --hostname=192.168.1.4 --ip=192.168.1.4 Duplicate flag 'quorum' on command line which is not very helpful. Current solution is to throw error if any duplication; over-write may make user confused about the result. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f802aec6a7768ce6f5aea7b437d980356a 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd656625b7fd414cacaa87f156c95df29438 Diff: https://reviews.apache.org/r/38259/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/ --- (Updated Sept. 13, 2015, 4:34 a.m.) Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park. Changes --- Update the code to overwrite env by cli Bugs: MESOS-3340 https://issues.apache.org/jira/browse/MESOS-3340 Repository: mesos Description --- Currently, it appears that re-defining a flag on the command-line that was already defined via a OS Env var (MESOS_*) causes the Master to fail with a not very helpful message. For example, if one has MESOS_QUORUM defined, this happens: $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 --hostname=192.168.1.4 --ip=192.168.1.4 Duplicate flag 'quorum' on command line which is not very helpful. Current solution is to throw error if any duplication; over-write may make user confused about the result. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 Diff: https://reviews.apache.org/r/38259/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines > > 600-601 > > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line600> > > > > the `+` should be at the end of the line (before the newline). > > Can you also please change the error message to: > > ``` > > "Command line flag '" + name + > > " duplicates environment variable definition with the same name" > > ``` > > also, please feel free to use all 80 columns. > > > > Now, an interesting question (for the shepherd/committer to decide) is > > whether we should just emit a WARN and have the command line (explicit) > > setting override the OS Env (implicit). > > > > Also, whether we should only WARN, but proceed, if the **values** are > > the same: > > > > ```MESOS_IP=127.0.0.1 > > ... --ip=127.0.0.1 > > ``` > > could (arguably) be considered a valid setting. > > > > As a general approach, I'd suggest however to stick with current > > behavior, to avoid breaking stuff that is currently working just fine (and > > relying on the executable to error out in case of misconfigurations). > > Klaus Ma wrote: > I used @haosdent's suggestion as follow, it looks better than mine :). > > return Error( > "Duplicate flag '" + name "' in command line with environment"); > > Your question is interesting, and I think it's a great improvement. Agree > with you to keep current behavior, we can log a new ticket to trace that > question. The behavior was updated to over-write environment by command line. - Klaus ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/#review98568 --- On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38259/ > --- > > (Updated Sept. 13, 2015, 5:30 a.m.) > > > Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park. > > > Bugs: MESOS-3340 > https://issues.apache.org/jira/browse/MESOS-3340 > > > Repository: mesos > > > Description > --- > > Currently, it appears that re-defining a flag on the command-line that was > already defined via a OS Env var (MESOS_*) causes the Master to fail with a > not very helpful message. > > For example, if one has MESOS_QUORUM defined, this happens: > > $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 > --hostname=192.168.1.4 --ip=192.168.1.4 > Duplicate flag 'quorum' on command line > > which is not very helpful. > > Current solution is to throw error if any duplication; over-write may make > user confused about the result. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 > > Diff: https://reviews.apache.org/r/38259/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38335/ --- Review request for mesos and Michael Park. Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 include/mesos/resources.hpp 6c3a065 src/common/http.cpp 9c0d31e src/master/http.cpp 73e8857 src/slave/containerizer/mesos/containerizer.cpp 1b83a87 src/tests/reservation_endpoints_tests.cpp dfab497 Diff: https://reviews.apache.org/r/38335/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review98818 --- src/Makefile.am (line 1463) <https://reviews.apache.org/r/37168/#comment155428> Done src/examples/dynamic_reservation_framework.cpp (line 40) <https://reviews.apache.org/r/37168/#comment155419> Done src/examples/dynamic_reservation_framework.cpp (line 49) <https://reviews.apache.org/r/37168/#comment155420> Done src/examples/dynamic_reservation_framework.cpp (line 180) <https://reviews.apache.org/r/37168/#comment155421> Done src/examples/dynamic_reservation_framework.cpp (line 183) <https://reviews.apache.org/r/37168/#comment155422> Done src/examples/dynamic_reservation_framework.cpp (line 184) <https://reviews.apache.org/r/37168/#comment155423> Done src/examples/dynamic_reservation_framework.cpp (line 281) <https://reviews.apache.org/r/37168/#comment155426> Done src/examples/dynamic_reservation_framework.cpp (line 283) <https://reviews.apache.org/r/37168/#comment155425> Done - Klaus Ma On Sept. 6, 2015, 4:11 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37168/ > --- > > (Updated Sept. 6, 2015, 4:11 a.m.) > > > Review request for mesos and Michael Park. > > > Bugs: MESOS-3063 > https://issues.apache.org/jira/browse/MESOS-3063 > > > Repository: mesos > > > Description > --- > > Provide example for dynamic reservation features. > > > Diffs > - > > src/Makefile.am 5fdca0f > src/examples/dynamic_reservation_framework.cpp PRE-CREATION > src/tests/dynamic_reservation_framework_test.sh PRE-CREATION > src/tests/examples_tests.cpp 3f56b30 > src/tests/flags.hpp 06da36d > src/tests/script.cpp bcc1fab > > Diff: https://reviews.apache.org/r/37168/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/ --- (Updated Sept. 14, 2015, 1:48 p.m.) Review request for mesos and Michael Park. Changes --- Address review comments Bugs: MESOS-3063 https://issues.apache.org/jira/browse/MESOS-3063 Repository: mesos Description --- Provide example for dynamic reservation features. Diffs (updated) - src/Makefile.am 8963cea src/examples/dynamic_reservation_framework.cpp PRE-CREATION src/tests/dynamic_reservation_framework_test.sh PRE-CREATION src/tests/examples_tests.cpp 3f56b30 src/tests/flags.hpp 06da36d src/tests/script.cpp bcc1fab Diff: https://reviews.apache.org/r/37168/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38253: Add containerId to ResourceUsage to enable QoS controller to target a container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38253/ --- (Updated Sept. 14, 2015, 10:41 a.m.) Review request for mesos and Niklas Nielsen. Summary (updated) - Add containerId to ResourceUsage to enable QoS controller to target a container Bugs: MESOS-2875 https://issues.apache.org/jira/browse/MESOS-2875 Repository: mesos Description --- We should ensure that we are addressing the container which the QoS controller intended to kill. Without this check, we may run into a scenario where the executor has terminated and one with the same id has started in the interim i.e. running in a different container than the one the QoS controller targeted. This most likely requires us to add containerId to the ResourceUsage message and encode the containerID in the QoS Correction message. Diffs - include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c include/mesos/slave/oversubscription.proto fa69a95689c8c75765f0800692655e8dde7dde33 src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 src/tests/oversubscription_tests.cpp 0c5edafc139d9bfb6806d007a0af85e80893bb1a Diff: https://reviews.apache.org/r/38253/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 569 > > <https://reviews.apache.org/r/38259/diff/1/?file=1067198#file1067198line569> > > > > Is it possible to change to foreach here? > > Klaus Ma wrote: > OK, let me address it. Just checked the code, we can not do that because 1) it's an array, 2) no requirement that argv is ended with NULL. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/#review98410 ------- On Sept. 11, 2015, 2:43 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38259/ > --- > > (Updated Sept. 11, 2015, 2:43 a.m.) > > > Review request for mesos, Bernd Mathiske and Marco Massenzio. > > > Bugs: MESOS-3340 > https://issues.apache.org/jira/browse/MESOS-3340 > > > Repository: mesos > > > Description > --- > > Currently, it appears that re-defining a flag on the command-line that was > already defined via a OS Env var (MESOS_*) causes the Master to fail with a > not very helpful message. > > For example, if one has MESOS_QUORUM defined, this happens: > > $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 > --hostname=192.168.1.4 --ip=192.168.1.4 > Duplicate flag 'quorum' on command line > > which is not very helpful. > > Current solution is to throw error if any duplication; over-write may make > user confused about the result. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp > 9da213f802aec6a7768ce6f5aea7b437d980356a > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp > ebf8cd656625b7fd414cacaa87f156c95df29438 > > Diff: https://reviews.apache.org/r/38259/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/ --- (Updated Sept. 11, 2015, 5:24 a.m.) Review request for mesos, Bernd Mathiske and Marco Massenzio. Bugs: MESOS-3340 https://issues.apache.org/jira/browse/MESOS-3340 Repository: mesos Description --- Currently, it appears that re-defining a flag on the command-line that was already defined via a OS Env var (MESOS_*) causes the Master to fail with a not very helpful message. For example, if one has MESOS_QUORUM defined, this happens: $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 --hostname=192.168.1.4 --ip=192.168.1.4 Duplicate flag 'quorum' on command line which is not very helpful. Current solution is to throw error if any duplication; over-write may make user confused about the result. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 Diff: https://reviews.apache.org/r/38259/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote: > > LGTM. But I not sure if this match @marco expect. Could you add he as > > reviewers? So currently your approach is show error message when > > `duplicates` is true. Use command line parameters when command line > > variables conflict with environment variables. I also suggest add comment > > and test cases about the last condition. > > haosdent huang wrote: > ``` > Use command line parameters when command line variables conflict with > environment variables. > ``` > -> > ``` > When duplicates is false, use command line parameters while ignore > environment variables. > ``` Add Macro as reviewer. Current solution is to throw error message and abort the command line if any duplicated items, so uer'll not be confused by overwrite. > On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 600 > > <https://reviews.apache.org/r/38259/diff/1/?file=1067198#file1067198line600> > > > > The style is not correct here. Please change to > > ``` > > return Error( > > "Duplicate flag '" + name "' in command line with > > environment"); > > ``` Sure, I'll correct it :). > On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 569 > > <https://reviews.apache.org/r/38259/diff/1/?file=1067198#file1067198line569> > > > > Is it possible to change to foreach here? OK, let me address it. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/#review98410 --- On Sept. 11, 2015, 2:43 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38259/ > --- > > (Updated Sept. 11, 2015, 2:43 a.m.) > > > Review request for mesos, Bernd Mathiske and Marco Massenzio. > > > Bugs: MESOS-3340 > https://issues.apache.org/jira/browse/MESOS-3340 > > > Repository: mesos > > > Description > --- > > Currently, it appears that re-defining a flag on the command-line that was > already defined via a OS Env var (MESOS_*) causes the Master to fail with a > not very helpful message. > > For example, if one has MESOS_QUORUM defined, this happens: > > $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 > --hostname=192.168.1.4 --ip=192.168.1.4 > Duplicate flag 'quorum' on command line > > which is not very helpful. > > Current solution is to throw error if any duplication; over-write may make > user confused about the result. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp > 9da213f802aec6a7768ce6f5aea7b437d980356a > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp > ebf8cd656625b7fd414cacaa87f156c95df29438 > > Diff: https://reviews.apache.org/r/38259/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/ --- (Updated Sept. 11, 2015, 9:54 a.m.) Review request for mesos, Bernd Mathiske and Marco Massenzio. Changes --- Address comments and propose a solution on duplicated logic Bugs: MESOS-3340 https://issues.apache.org/jira/browse/MESOS-3340 Repository: mesos Description --- Currently, it appears that re-defining a flag on the command-line that was already defined via a OS Env var (MESOS_*) causes the Master to fail with a not very helpful message. For example, if one has MESOS_QUORUM defined, this happens: $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 --hostname=192.168.1.4 --ip=192.168.1.4 Duplicate flag 'quorum' on command line which is not very helpful. Current solution is to throw error if any duplication; over-write may make user confused about the result. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 Diff: https://reviews.apache.org/r/38259/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 605
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line605>
> >
> > nit: please retain `on` as in the original message.
Addressed
> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines
> > 600-601
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line600>
> >
> > the `+` should be at the end of the line (before the newline).
> > Can you also please change the error message to:
> > ```
> > "Command line flag '" + name +
> > " duplicates environment variable definition with the same name"
> > ```
> > also, please feel free to use all 80 columns.
> >
> > Now, an interesting question (for the shepherd/committer to decide) is
> > whether we should just emit a WARN and have the command line (explicit)
> > setting override the OS Env (implicit).
> >
> > Also, whether we should only WARN, but proceed, if the **values** are
> > the same:
> >
> > ```MESOS_IP=127.0.0.1
> > ... --ip=127.0.0.1
> > ```
> > could (arguably) be considered a valid setting.
> >
> > As a general approach, I'd suggest however to stick with current
> > behavior, to avoid breaking stuff that is currently working just fine (and
> > relying on the executable to error out in case of misconfigurations).
I used @haosdent's suggestion as follow, it looks better than mine :).
return Error(
"Duplicate flag '" + name "' in command line with environment");
Your question is interesting, and I think it's a great improvement. Agree with
you to keep current behavior, we can log a new ticket to trace that question.
> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines
> > 672-681
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line672>
> >
> > would it be possible to factor out all this into a helper method?
> > It's essentialy a copy & paste of the same code above.
> >
> > so long as it was only a couple of lines of code (maybe) it made sense
> > to repeat it - here, it seems redundant.
> >
> > (and, obviously, same comments apply)
I'm thinking to define a const massage format for re-usage; but not sure where
to add such a const format. For example:
const string dup_err_msg("Duplicate flag '%s' in command line with
environment"); // No sure where to put this const string :(.
...
return Error(strings::format(dup_err_msg, name));
And re-check those two log/function, it seems we can re-use some logic: in
`FlagsBase::load(... const char* const* argv ...)`, dump `argv` and call
`FlagsBase::load(... char*** argv ...)` to load the flags, the args that are
not a flag.
On Sept. 11, 2015, 6:56 a.m., Klaus Ma wrote:
> > Thanks for doing this!
> > A few comments, but generally good.
> >
> > Not sure if you have a shepherd for this - if not, please let me know and I
> > can help find one.
No shepherd now; that'll be great if you or Bernd can find a shepherd for this
one.
- Klaus
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38259/#review98568
---
On Sept. 11, 2015, 5:24 a.m., Klaus Ma wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
>
> (Updated Sept. 11, 2015, 5:24 a.m.)
>
>
> Review request for mesos, Bernd Mathiske and Marco Massenzio.
>
>
> Bugs: MESOS-3340
> https://issues.apache.org/jira/browse/MESOS-3340
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Currently, it appears that re-defining a flag on the command-line that was
> already defined via a OS Env var (MESOS_*) causes the Master to fail with a
> not very helpful message.
>
> For example, if one has MESOS_QUORUM defined, this happens:
>
> $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1
> --hostname=192.168.1.4 --ip=192.168.1.4
> Duplicate flag 'quorum' on command line
>
> which is not very helpful.
>
> Current solution is to throw error if any duplication; over-write may make
> user confused about the result.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6
>
> Diff: https://reviews.apache.org/r/38259/diff/
>
>
> Testing
> ---
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
> On Sept. 15, 2015, 4:54 a.m., Marco Massenzio wrote: > > LGTM > > Can you please add a test with a few vars defined in Env and then in Cmd > > Line too, and verify that it "works as intended"? > > > > Thanks for bearing with me and changing your code. > > I think it looks clean and nice now :) There used to be a test case on "DuplicatesFromEnvironment", I updated it according to our design behavior: overwrite env by cli. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38259/#review98983 ------- On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38259/ > --- > > (Updated Sept. 13, 2015, 5:30 a.m.) > > > Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park. > > > Bugs: MESOS-3340 > https://issues.apache.org/jira/browse/MESOS-3340 > > > Repository: mesos > > > Description > --- > > Currently, it appears that re-defining a flag on the command-line that was > already defined via a OS Env var (MESOS_*) causes the Master to fail with a > not very helpful message. > > For example, if one has MESOS_QUORUM defined, this happens: > > $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 > --hostname=192.168.1.4 --ip=192.168.1.4 > Duplicate flag 'quorum' on command line > > which is not very helpful. > > Current solution is to throw error if any duplication; over-write may make > user confused about the result. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 > > Diff: https://reviews.apache.org/r/38259/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38102: MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random)
> On Sept. 14, 2015, 11:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp, line 31 > > <https://reviews.apache.org/r/38102/diff/1/?file=1063282#file1063282line31> > > > > Please use THREAD_LOCAL from stout/thread_local.hpp and store a heap > > object as done in all of our other uses of THREAD_LOCAL. Addressed :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38102/#review98950 --- On Sept. 15, 2015, 9:39 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38102/ > --- > > (Updated Sept. 15, 2015, 9:39 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3046 > https://issues.apache.org/jira/browse/MESOS-3046 > > > Repository: mesos > > > Description > --- > > __Phenomenon:__ > Performance downgrade > > __Root Cause:__ > stout's UUID abstraction is re-seeding the random generator during each call > to UUID::random(), which is really expensive. > > __Solution:__ > Seeding the random generator only once per thread. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp e90dabb > > Diff: https://reviews.apache.org/r/38102/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 38102: MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38102/ --- (Updated Sept. 15, 2015, 9:39 a.m.) Review request for mesos and Ben Mahler. Changes --- Address comments Bugs: MESOS-3046 https://issues.apache.org/jira/browse/MESOS-3046 Repository: mesos Description --- __Phenomenon:__ Performance downgrade __Root Cause:__ stout's UUID abstraction is re-seeding the random generator during each call to UUID::random(), which is really expensive. __Solution:__ Seeding the random generator only once per thread. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp e90dabb Diff: https://reviews.apache.org/r/38102/diff/ Testing --- make make check Thanks, Klaus Ma
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/ --- (Updated Sept. 15, 2015, 9:43 a.m.) Review request for mesos, Alexander Rukletsov and Michael Park. Changes --- Address comments Bugs: MESOS-3405 https://issues.apache.org/jira/browse/MESOS-3405 Repository: mesos Description --- Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a `google::protobuf::Message` into a `JSON::Object`. We should add the support for `google::protobuf::RepeatedPtrField` by introducing overloaded functions. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 Diff: https://reviews.apache.org/r/38342/diff/ Testing --- cd 3rdparty/libprocess/3rdparty/stout ./boostrap ./configure make Thanks, Klaus Ma
Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
> On Sept. 14, 2015, 5:39 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 112 > > <https://reviews.apache.org/r/38342/diff/1/?file=1072088#file1072088line112> > > > > Let's `reserve()` first. Addressed, thanks :). - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38342/#review98875 --- On Sept. 15, 2015, 9:43 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38342/ > --- > > (Updated Sept. 15, 2015, 9:43 a.m.) > > > Review request for mesos, Alexander Rukletsov and Michael Park. > > > Bugs: MESOS-3405 > https://issues.apache.org/jira/browse/MESOS-3405 > > > Repository: mesos > > > Description > --- > > Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which > converts a `google::protobuf::Message` into a `JSON::Object`. > We should add the support for `google::protobuf::RepeatedPtrField` by > introducing overloaded functions. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf > 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 > > Diff: https://reviews.apache.org/r/38342/diff/ > > > Testing > --- > > cd 3rdparty/libprocess/3rdparty/stout > ./boostrap > ./configure > make > > > Thanks, > > Klaus Ma > >
