You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Yongqiao Wang <yq...@cn.ibm.com> on 2016/01/05 06:46:39 UTC
Re: Review Request 41672: Test case(s) for weights + allocation
behavior
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 5:46 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Update this patch due to the UpdateWeight() interface change.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description
-------
Test case(s) for weights + allocation behavior
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behavior
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113990
-----------------------------------------------------------
Bad patch!
Reviews applied: [41597, 41672]
Failed command: ./support/apply-review.sh -n -r 41672
Error:
2016-01-12 14:30:48 URL:https://reviews.apache.org/r/41672/diff/raw/ [7731/7731] -> "41672.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.
- Mesos ReviewBot
On Jan. 12, 2016, 9:35 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 9:35 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 15, 2016, 1:34 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2093-2096
> > <https://reviews.apache.org/r/41672/diff/7/?file=1194005#file1194005line2093>
> >
> > We rarely (only 67 times) use `unsigned`. Usually it's `int` (909 times) for signed values (or values where sign is irrelevant), or `uint##_t` for unsigned values of a particular size (e.g. 401 `uint64_t`s).
>
> Yongqiao Wang wrote:
> 'unsigned' is widely used in this file, so I choose to use it to keep consistence in the same file.
Fair enough. With 15 uses, it's definitely the biggest user of unsigned.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114688
-----------------------------------------------------------
On Jan. 12, 2016, 6:56 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 6:56 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Jan. 15, 2016, 9:34 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2093-2096
> > <https://reviews.apache.org/r/41672/diff/7/?file=1194005#file1194005line2093>
> >
> > We rarely (only 67 times) use `unsigned`. Usually it's `int` (909 times) for signed values (or values where sign is irrelevant), or `uint##_t` for unsigned values of a particular size (e.g. 401 `uint64_t`s).
'unsigned' is widely used in this file, so I choose to use it to keep consistence in the same file.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114688
-----------------------------------------------------------
On Jan. 12, 2016, 2:56 p.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 2:56 p.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114688
-----------------------------------------------------------
Looking good. A few code changes, but mostly grammar nitpicking/suggestions.
src/tests/hierarchical_allocator_tests.cpp (line 2146)
<https://reviews.apache.org/r/41672/#comment175551>
s/which contains/with/
src/tests/hierarchical_allocator_tests.cpp (line 2175)
<https://reviews.apache.org/r/41672/#comment175550>
You could instead say `QUADRUPLE`, but I actually like the sound of `FOURFOLD`.
src/tests/hierarchical_allocator_tests.cpp (line 2220)
<https://reviews.apache.org/r/41672/#comment175552>
s/there/therefore/
src/tests/hierarchical_allocator_tests.cpp (line 2221)
<https://reviews.apache.org/r/41672/#comment175553>
s/ratio is/ratio of/
src/tests/hierarchical_allocator_tests.cpp (line 224)
<https://reviews.apache.org/r/41672/#comment175554>
`static`? Or is that so obvious the compiler will pick it up on its own?
src/tests/hierarchical_allocator_tests.cpp (lines 2093 - 2096)
<https://reviews.apache.org/r/41672/#comment175555>
We rarely (only 67 times) use `unsigned`. Usually it's `int` (909 times) for signed values (or values where sign is irrelevant), or `uint##_t` for unsigned values of a particular size (e.g. 401 `uint64_t`s).
src/tests/hierarchical_allocator_tests.cpp (line 2100)
<https://reviews.apache.org/r/41672/#comment175556>
const? (since you're going ALL_CAPS)
Actually, why is this even a variable if it's only used in one place below?
src/tests/hierarchical_allocator_tests.cpp (line 2118)
<https://reviews.apache.org/r/41672/#comment175557>
s/have/having/
src/tests/hierarchical_allocator_tests.cpp (line 2136)
<https://reviews.apache.org/r/41672/#comment175558>
Because each framework's role has a weight of 1.0 by default,...
src/tests/hierarchical_allocator_tests.cpp (line 2139)
<https://reviews.apache.org/r/41672/#comment175559>
Let's define all of these together up at the top.
src/tests/hierarchical_allocator_tests.cpp (line 2151)
<https://reviews.apache.org/r/41672/#comment175560>
s/for the next time allocation/so they can be offered again next time./
src/tests/hierarchical_allocator_tests.cpp (line 2171)
<https://reviews.apache.org/r/41672/#comment175561>
Now that the frameworks' weights are 1:2,...
src/tests/hierarchical_allocator_tests.cpp (line 2172)
<https://reviews.apache.org/r/41672/#comment175562>
s/all resources are also offered by 1:2/resources are offered with a ratio of 1:2/
- Adam B
On Jan. 12, 2016, 6:56 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 6:56 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114757
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41597, 41672]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 15, 2016, 10:47 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 10:47 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
> Testing Done:
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
> Bugs closed: MESOS-4200
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Neil Conway <ne...@gmail.com>.
> On Jan. 18, 2016, 8:29 a.m., Neil Conway wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2096-2101
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2096>
> >
> > I think we should make `i` an `int`, not `unsigned` -- unless we depend on the semantics of unsigned, which we don't. Using `int` is more idiomatic and consistent with the rest of the code base.
>
> Yongqiao Wang wrote:
> 'unsigned' is widely used in this file, so I choose to use it to keep consistence in the same file.
We should probably change the file to be consistent with the rest of the project. We can do that separately/later, though.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114966
-----------------------------------------------------------
On Jan. 18, 2016, 8:54 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 8:54 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Jan. 18, 2016, 8:29 a.m., Neil Conway wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2096-2101
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2096>
> >
> > I think we should make `i` an `int`, not `unsigned` -- unless we depend on the semantics of unsigned, which we don't. Using `int` is more idiomatic and consistent with the rest of the code base.
'unsigned' is widely used in this file, so I choose to use it to keep consistence in the same file.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114966
-----------------------------------------------------------
On Jan. 18, 2016, 8:22 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 8:22 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114966
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 2093)
<https://reviews.apache.org/r/41672/#comment175808>
Making this a separate variable doesn't seem useful.
src/tests/hierarchical_allocator_tests.cpp (lines 2096 - 2101)
<https://reviews.apache.org/r/41672/#comment175803>
I think we should make `i` an `int`, not `unsigned` -- unless we depend on the semantics of unsigned, which we don't. Using `int` is more idiomatic and consistent with the rest of the code base.
src/tests/hierarchical_allocator_tests.cpp (line 2101)
<https://reviews.apache.org/r/41672/#comment175802>
Seems like we can use `foreach` here.
src/tests/hierarchical_allocator_tests.cpp (line 2216)
<https://reviews.apache.org/r/41672/#comment175804>
"Adding a framework"
src/tests/hierarchical_allocator_tests.cpp (line 2228)
<https://reviews.apache.org/r/41672/#comment175806>
Whitespace after the `if`.
- Neil Conway
On Jan. 18, 2016, 8:22 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 8:22 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review115416
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41597, 41672]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 8:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2423-2433
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2423>
> >
> > Will it be cleaner to put resources from the allocation into a hashmap by framework id? I think this way you can even get rid of the loop.
> >
> > Same suggestions for the section below.
>
> Yongqiao Wang wrote:
> Personallly, the current implementation is cleaner, if we using a loop for this, we also need to check the framework Id in the loop due to there allocation size and resources are different.
>
> Adam B wrote:
> Not to mention that you still have to call `allocations.get()` each time. And you'd have to have two hashmaps, one for allocation size and another for resources, or create a struct to hold both. I don't see how you could do it without the loop unless you unroll the loop or hide it behind a lambda. I don't think we need to change the implementation here. Dropping the issue.
For posterity, I've explained what I meant in https://reviews.apache.org/r/43824/#comment183415
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------
On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 8:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 19, 2016, 1:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2325
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2325>
> >
> > Why post-increment? Is it consistent to the codebase?
Looks like post-increment wins in this file (14:6) and across all of /tests (76:15), but by a smaller margin in all of /src (535:368).
I know there are times when pre/post matters when incrementing a complex object, but for simple ints it doesn't matter.
Besides, the language is named "C++" not "++C" damnit!
> On Feb. 19, 2016, 1:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2338
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2338>
> >
> > Backticks instead of single quotes, please! Here and everywhere.
>
> Yongqiao Wang wrote:
> I find single quote?apostrophe and backticks are all used in our comments, You can check this file and others like `master_quota_tests.cpp` `master_maintenance_tests.cpp`, etc. Cloud you tell me what is the rule for using them in comments, I will follow up later. Thanks.
Looks like single quotes are more popular than backticks in this file (99:71) and /tests (1162:293). As far as I know, backticks really only show up differently in markdown and doxygen. But our http://mesos.apache.org/documentation/latest/c++-style-guide/ says "Use backticks when quoting code excerpts or object/variable/function names." without stating whether that applies only to doxygenized comments.
Now, 'role1' is a string value and not an actual class/variable/function/test name, so it could stay in single quotes (or double quotes), but anything referencing a named piece of code should use backticks. Doesn't look like there's anything like that in this patch, so I'm dropping the issue.
> On Feb. 19, 2016, 1:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2373
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2373>
> >
> > You can put framework id's into a `std::set` instead. Given there are two allocations, there is a guarantee, that there will be no attempts to put the same framework into the set twice.
>
> Yongqiao Wang wrote:
> In the current implementation, we only need to check the allocation size for each framework, it does not care the size of whole contianer (currentt is hashmap), so I think use hashmap can meet the current requirement.
I think he's saying you can use a `std::set<FrameworkID> allocatedFrameworks;` and inside your loop do: `allocatedFrameworks.insert(allocation.get().frameworkId);`
and verify the allocations afterwards with: `EXPECT_EQ(expectedFrameworksSet, allocatedFrameworks);`
> On Feb. 19, 2016, 1:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2402-2403
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2402>
> >
> > If you use `set` as proposed above, you can check set against set here.
>
> Yongqiao Wang wrote:
> Sorry to do not understand your suggestion here, current using hashmap and use frameworkid as it's key, here to check value for allocation number for each framework.
I think he's saying rather than counting that allocation number is always 1 for each of them, you could just add each frameworkId to a set and then at the end you verify that the test `set` matches your expected `set`.
> On Feb. 19, 2016, 1:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2423-2433
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2423>
> >
> > Will it be cleaner to put resources from the allocation into a hashmap by framework id? I think this way you can even get rid of the loop.
> >
> > Same suggestions for the section below.
>
> Yongqiao Wang wrote:
> Personallly, the current implementation is cleaner, if we using a loop for this, we also need to check the framework Id in the loop due to there allocation size and resources are different.
Not to mention that you still have to call `allocations.get()` each time. And you'd have to have two hashmaps, one for allocation size and another for resources, or create a struct to hold both. I don't see how you could do it without the loop unless you unroll the loop or hide it behind a lambda. I don't think we need to change the implementation here. Dropping the issue.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------
On Jan. 20, 2016, 12:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 12:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 19, 2016, 1:31 a.m., Alexander Rukletsov wrote:
> >
Yongqiao, since Alex didn't get his review in before I committed the patch, could you create a new patch addressing his feedback and link to it from the comments here? Thanks.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------
On Jan. 20, 2016, 12:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 12:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> >
>
> Adam B wrote:
> Yongqiao, since Alex didn't get his review in before I committed the patch, could you create a new patch addressing his feedback and link to it from the comments here? Thanks.
Thanks Adam and Alex, I have posted patch https://reviews.apache.org/r/43824/ to address the comments as below.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2338
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2338>
> >
> > Backticks instead of single quotes, please! Here and everywhere.
I find single quote?apostrophe and backticks are all used in our comments, You can check this file and others like `master_quota_tests.cpp` `master_maintenance_tests.cpp`, etc. Cloud you tell me what is the rule for using them in comments, I will follow up later. Thanks.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2423-2433
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2423>
> >
> > Will it be cleaner to put resources from the allocation into a hashmap by framework id? I think this way you can even get rid of the loop.
> >
> > Same suggestions for the section below.
Personallly, the current implementation is cleaner, if we using a loop for this, we also need to check the framework Id in the loop due to there allocation size and resources are different.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2373
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2373>
> >
> > You can put framework id's into a `std::set` instead. Given there are two allocations, there is a guarantee, that there will be no attempts to put the same framework into the set twice.
In the current implementation, we only need to check the allocation size for each framework, it does not care the size of whole contianer (currentt is hashmap), so I think use hashmap can meet the current requirement.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2402-2403
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2402>
> >
> > If you use `set` as proposed above, you can check set against set here.
Sorry to do not understand your suggestion here, current using hashmap and use frameworkid as it's key, here to check value for allocation number for each framework.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------
On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 8:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2373
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2373>
> >
> > You can put framework id's into a `std::set` instead. Given there are two allocations, there is a guarantee, that there will be no attempts to put the same framework into the set twice.
>
> Yongqiao Wang wrote:
> In the current implementation, we only need to check the allocation size for each framework, it does not care the size of whole contianer (currentt is hashmap), so I think use hashmap can meet the current requirement.
>
> Adam B wrote:
> I think he's saying you can use a `std::set<FrameworkID> allocatedFrameworks;` and inside your loop do: `allocatedFrameworks.insert(allocation.get().frameworkId);`
> and verify the allocations afterwards with: `EXPECT_EQ(expectedFrameworksSet, allocatedFrameworks);`
Have changed `hashmap` to `std::set`
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------
On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 8:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 2307)
<https://reviews.apache.org/r/41672/#comment181212>
Let's abolish slavery in this test : ). Here and below in comments and variable namings.
src/tests/hierarchical_allocator_tests.cpp (lines 2320 - 2324)
<https://reviews.apache.org/r/41672/#comment181213>
This is not related to "register agents with the same resources" section, right? Let's move it up and write why this is necessary.
src/tests/hierarchical_allocator_tests.cpp (line 2325)
<https://reviews.apache.org/r/41672/#comment181223>
Why post-increment? Is it consistent to the codebase?
src/tests/hierarchical_allocator_tests.cpp (line 2329)
<https://reviews.apache.org/r/41672/#comment181215>
Why not merging this loop with the previous one?
src/tests/hierarchical_allocator_tests.cpp (line 2335)
<https://reviews.apache.org/r/41672/#comment181214>
`{}` will do the job.
src/tests/hierarchical_allocator_tests.cpp (line 2337)
<https://reviews.apache.org/r/41672/#comment181217>
Here is a good place to insert a comment describing the cluster state. Look for an example in other tests.
src/tests/hierarchical_allocator_tests.cpp (line 2338)
<https://reviews.apache.org/r/41672/#comment181219>
Backticks instead of single quotes, please! Here and everywhere.
src/tests/hierarchical_allocator_tests.cpp (line 2342)
<https://reviews.apache.org/r/41672/#comment181220>
`{}` will do the job. Here and below.
src/tests/hierarchical_allocator_tests.cpp (lines 2350 - 2354)
<https://reviews.apache.org/r/41672/#comment181221>
How about separating this in two sections: check allocation state and recovering resources?
Between these sections you can insert a comment describing the cluster state (I've mentioned that above already).
src/tests/hierarchical_allocator_tests.cpp (lines 2371 - 2397)
<https://reviews.apache.org/r/41672/#comment181225>
You can wrap this block in curly braces and avoid adding numerals to variable names. Same for the block below.
src/tests/hierarchical_allocator_tests.cpp (line 2373)
<https://reviews.apache.org/r/41672/#comment181229>
You can put framework id's into a `std::set` instead. Given there are two allocations, there is a guarantee, that there will be no attempts to put the same framework into the set twice.
src/tests/hierarchical_allocator_tests.cpp (line 2374)
<https://reviews.apache.org/r/41672/#comment181224>
Let's pull advancing clock above so that it's not lost between lines.
src/tests/hierarchical_allocator_tests.cpp (lines 2402 - 2403)
<https://reviews.apache.org/r/41672/#comment181230>
If you use `set` as proposed above, you can check set against set here.
src/tests/hierarchical_allocator_tests.cpp (lines 2413 - 2414)
<https://reviews.apache.org/r/41672/#comment181226>
You can avoid this by wrapping this section in curly braces.
src/tests/hierarchical_allocator_tests.cpp (lines 2423 - 2433)
<https://reviews.apache.org/r/41672/#comment181231>
Will it be cleaner to put resources from the allocation into a hashmap by framework id? I think this way you can even get rid of the loop.
Same suggestions for the section below.
- Alexander Rukletsov
On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 8:36 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 20, 2016, 8:36 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Addressed comments of Adam.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description
-------
Test case(s) for weights + allocation behaviour.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review115364
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 2352)
<https://reviews.apache.org/r/41672/#comment176348>
s/registering/registered/
src/tests/hierarchical_allocator_tests.cpp (lines 2357 - 2358)
<https://reviews.apache.org/r/41672/#comment176349>
Also `EXPECT_EQ(allocation.get().frameworkId, framework1.id());` and `ASSERT_EQ(6u, allocation.get().resources.size());` to match what you're doing elsewhere.
src/tests/hierarchical_allocator_tests.cpp (line 2444)
<https://reviews.apache.org/r/41672/#comment176350>
s/have possessed all resources/sum to the total resources/
src/tests/hierarchical_allocator_tests.cpp (line 2490)
<https://reviews.apache.org/r/41672/#comment176351>
s/have possessed all resources/sum to the total resources/
- Adam B
On Jan. 19, 2016, 10:49 p.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 10:49 p.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 20, 2016, 6:49 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Addressed comments of Adam.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description
-------
Test case(s) for weights + allocation behaviour.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review114979
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41597, 41672]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 18, 2016, 8:54 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 8:54 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Jan. 20, 2016, 6:09 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2177-2178
> > <https://reviews.apache.org/r/41672/diff/10/?file=1199918#file1199918line2177>
> >
> > Where do you verify that role2 doesn't get any offers? Shouldn't you verify that `allocations.get()` returns a single allocation of (6) TOTAL_RESOURCES all associated with role1?
Thanks Adam, I have enhanced the comment, and verified it.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review115354
-----------------------------------------------------------
On Jan. 20, 2016, 6:49 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 6:49 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review115354
-----------------------------------------------------------
Ship it!
Looks great! Doesn't look like you're verifying the first offer cycle in your test, but that should be a quick fix.
src/tests/hierarchical_allocator_tests.cpp (line 2222)
<https://reviews.apache.org/r/41672/#comment176334>
Please add a blank line before this (or any comment line).
s/have possessed all resources/sum to the total resources/
src/tests/hierarchical_allocator_tests.cpp (line 2223)
<https://reviews.apache.org/r/41672/#comment176335>
s/there is/there are/
src/tests/hierarchical_allocator_tests.cpp (lines 2268 - 2269)
<https://reviews.apache.org/r/41672/#comment176336>
Same as above
src/tests/hierarchical_allocator_tests.cpp (line 2312)
<https://reviews.apache.org/r/41672/#comment176337>
Same as above
src/tests/hierarchical_allocator_tests.cpp (lines 2163 - 2167)
<https://reviews.apache.org/r/41672/#comment176339>
These should be indented 4 spaces instead of 2
src/tests/hierarchical_allocator_tests.cpp (lines 2177 - 2178)
<https://reviews.apache.org/r/41672/#comment176341>
Where do you verify that role2 doesn't get any offers? Shouldn't you verify that `allocations.get()` returns a single allocation of (6) TOTAL_RESOURCES all associated with role1?
- Adam B
On Jan. 18, 2016, 12:54 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 12:54 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 18, 2016, 8:54 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Addressed the comments of Neil.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description (updated)
-------
Test case(s) for weights + allocation behaviour.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 18, 2016, 8:22 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Check to ensure that these allocations have possessed all resources, this check can ensure there is no any other allocations in this case.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description (updated)
-------
Test case(s) for weights + allocation behavior
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/41672/diff/
Testing (updated)
-------
Make check done:
$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 10:47 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Addressed comments of adam.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description (updated)
-------
Test case(s) for weights + allocation behavior
Testing Done:
Make check done:
Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Bugs closed: MESOS-4200
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 12, 2016, 2:56 p.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Summary (updated)
-----------------
Test case(s) for weights + allocation behaviour.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description
-------
Test case(s) for weights + allocation behavior
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behavior
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/
-----------------------------------------------------------
(Updated Jan. 12, 2016, 9:35 a.m.)
Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
Changes
-------
Addressed comments of Adam.
Bugs: MESOS-4200
https://issues.apache.org/jira/browse/MESOS-4200
Repository: mesos
Description
-------
Test case(s) for weights + allocation behavior
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
Diff: https://reviews.apache.org/r/41672/diff/
Testing
-------
Make check done:
Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
Source directory: /Users/yqwyq/Desktop/mesos
Build directory: /Users/yqwyq/Desktop/mesos/build
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HierarchicalAllocatorTest
[ RUN ] HierarchicalAllocatorTest.UpdateWeight
[ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
[----------] 1 test from HierarchicalAllocatorTest (87 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (176 ms total)
[ PASSED ] 1 test.
Thanks,
Yongqiao Wang
Re: Review Request 41672: Test case(s) for weights + allocation
behavior
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Jan. 11, 2016, 10:13 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2218-2219
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2218>
> >
> > Why hardcode 3? I'd think you could just run a while loop and actually assert the size that you think the allocations hashmap will be. (And what key will be in each entry.)
allocations is a process::Queue<Allocation>, and I can not get its size due to there is no interface can do this. Could you give me a detailed description for your suggestions? Thanks.
In addition, I can make sure there are 3 allocations because in my test case, there are 4 agents and 3 frameworks, and their weights are 3:1:2, so according to the fair share, each framework can get one allocation at least.
> On Jan. 11, 2016, 10:13 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2095
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2095>
> >
> > When did we start initializing string constants with braces instead of '='s? What's the benefit?
I refer the quota-related code to use {""} (such as master_quota_tests.cpp, and quota tests in this file), actually, I am not sure the difference between them, but in C++ standard library, it uses '=', so I have updated my code to use '=' now.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113711
-----------------------------------------------------------
On Jan. 5, 2016, 5:46 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 5:46 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Jan. 11, 2016, 10:13 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2218-2219
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2218>
> >
> > Why hardcode 3? I'd think you could just run a while loop and actually assert the size that you think the allocations hashmap will be. (And what key will be in each entry.)
>
> Yongqiao Wang wrote:
> allocations is a process::Queue<Allocation>, and I can not get its size due to there is no interface can do this. Could you give me a detailed description for your suggestions? Thanks.
>
> In addition, I can make sure there are 3 allocations because in my test case, there are 4 agents and 3 frameworks, and their weights are 3:1:2, so according to the fair share, each framework can get one allocation at least.
>
> Adam B wrote:
> Fair point, and even if you did a `while(allocations.get() != something)`, you'd actually be getting a future that your `while` condition would have to AWAIT before it could evaluate. Messy.
> Rather, you're iterating 3 times and AWAITing each time, therefore asserting that there are at least 3 allocations. To prove that there are exactly 3 (or 2 in the other cases), can you `AWAIT_FAILED(allocations.get())` after your loops? I'm not sure if the `Clock` would work for/against you there, especially since you're recovering resources inside the loop instead of after it.
> Looks like `process::queue::get()` returns a READY future immediately if the offer queue isn't empty, so maybe you can get away with `ASSERT_FALSE(allocation.isReady());` after the loop.
I tested found that ASSERT_FALSE(allocations.get().isReady()) does not work, because a new promise will be created when call allocations.get().
I have done this by checking whether the total resources have been allocated to those allocations, if yes, then we can be sure there are only 3 or 2 allocations in those cases due to no resources for any other allocation. you can refer to the code diff for the detais, thx.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113711
-----------------------------------------------------------
On Jan. 15, 2016, 10:47 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 10:47 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
> Testing Done:
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
> Bugs closed: MESOS-4200
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 11, 2016, 2:13 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2218-2219
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2218>
> >
> > Why hardcode 3? I'd think you could just run a while loop and actually assert the size that you think the allocations hashmap will be. (And what key will be in each entry.)
>
> Yongqiao Wang wrote:
> allocations is a process::Queue<Allocation>, and I can not get its size due to there is no interface can do this. Could you give me a detailed description for your suggestions? Thanks.
>
> In addition, I can make sure there are 3 allocations because in my test case, there are 4 agents and 3 frameworks, and their weights are 3:1:2, so according to the fair share, each framework can get one allocation at least.
>
> Adam B wrote:
> Fair point, and even if you did a `while(allocations.get() != something)`, you'd actually be getting a future that your `while` condition would have to AWAIT before it could evaluate. Messy.
> Rather, you're iterating 3 times and AWAITing each time, therefore asserting that there are at least 3 allocations. To prove that there are exactly 3 (or 2 in the other cases), can you `AWAIT_FAILED(allocations.get())` after your loops? I'm not sure if the `Clock` would work for/against you there, especially since you're recovering resources inside the loop instead of after it.
> Looks like `process::queue::get()` returns a READY future immediately if the offer queue isn't empty, so maybe you can get away with `ASSERT_FALSE(allocation.isReady());` after the loop.
>
> Yongqiao Wang wrote:
> I tested found that ASSERT_FALSE(allocations.get().isReady()) does not work, because a new promise will be created when call allocations.get().
>
> I have done this by checking whether the total resources have been allocated to those allocations, if yes, then we can be sure there are only 3 or 2 allocations in those cases due to no resources for any other allocation. you can refer to the code diff for the detais, thx.
Good enough for me. Dropping the issue. Thanks for investigating.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113711
-----------------------------------------------------------
On Jan. 18, 2016, 12:54 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 12:54 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behaviour.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 11, 2016, 2:13 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2095
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2095>
> >
> > When did we start initializing string constants with braces instead of '='s? What's the benefit?
>
> Yongqiao Wang wrote:
> I refer the quota-related code to use {""} (such as master_quota_tests.cpp, and quota tests in this file), actually, I am not sure the difference between them, but in C++ standard library, it uses '=', so I have updated my code to use '=' now.
Thank you. I'm not sure I agree with its use in the quota code either, but that's a separate discussion. Moral of the story: don't just copy 1 example, since it could be a flawed example. Examine 2-3 if possible to see where they're the same and where they differ, then go with the most sensible/common version.
> On Jan. 11, 2016, 2:13 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2218-2219
> > <https://reviews.apache.org/r/41672/diff/6/?file=1181569#file1181569line2218>
> >
> > Why hardcode 3? I'd think you could just run a while loop and actually assert the size that you think the allocations hashmap will be. (And what key will be in each entry.)
>
> Yongqiao Wang wrote:
> allocations is a process::Queue<Allocation>, and I can not get its size due to there is no interface can do this. Could you give me a detailed description for your suggestions? Thanks.
>
> In addition, I can make sure there are 3 allocations because in my test case, there are 4 agents and 3 frameworks, and their weights are 3:1:2, so according to the fair share, each framework can get one allocation at least.
Fair point, and even if you did a `while(allocations.get() != something)`, you'd actually be getting a future that your `while` condition would have to AWAIT before it could evaluate. Messy.
Rather, you're iterating 3 times and AWAITing each time, therefore asserting that there are at least 3 allocations. To prove that there are exactly 3 (or 2 in the other cases), can you `AWAIT_FAILED(allocations.get())` after your loops? I'm not sure if the `Clock` would work for/against you there, especially since you're recovering resources inside the loop instead of after it.
Looks like `process::queue::get()` returns a READY future immediately if the offer queue isn't empty, so maybe you can get away with `ASSERT_FALSE(allocation.isReady());` after the loop.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113711
-----------------------------------------------------------
On Jan. 12, 2016, 6:56 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 6:56 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behavior
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113711
-----------------------------------------------------------
Looking good. Just some minor suggestions here and there.
src/tests/hierarchical_allocator_tests.cpp (line 224)
<https://reviews.apache.org/r/41672/#comment174517>
No need to make a `const double` since it's passed by value anyway. Just use `double weight`.
src/tests/hierarchical_allocator_tests.cpp (line 2095)
<https://reviews.apache.org/r/41672/#comment174518>
When did we start initializing string constants with braces instead of '='s? What's the benefit?
src/tests/hierarchical_allocator_tests.cpp (lines 2096 - 2101)
<https://reviews.apache.org/r/41672/#comment174520>
Please be consistent in your use of `i++` vs. `++i` in scalar loops where the order doesn't matter. Looks like we tend to use `i++` much more often.
src/tests/hierarchical_allocator_tests.cpp (line 2110)
<https://reviews.apache.org/r/41672/#comment174521>
s/register/registers/g
s/using/uses/g
src/tests/hierarchical_allocator_tests.cpp (line 2111)
<https://reviews.apache.org/r/41672/#comment174522>
s/allocated/offered/
src/tests/hierarchical_allocator_tests.cpp (line 2118)
<https://reviews.apache.org/r/41672/#comment174523>
s/have been allocated/have outstanding offers/
src/tests/hierarchical_allocator_tests.cpp (lines 2125 - 2126)
<https://reviews.apache.org/r/41672/#comment174525>
If all parameters don't fit on one line, then each parameter should get its own line. Please wrap `allocation.get().resources` onto its own line.
src/tests/hierarchical_allocator_tests.cpp (line 2126)
<https://reviews.apache.org/r/41672/#comment174526>
How do you know that the `allocation` Future is ready yet? Better AWAIT_READY?
src/tests/hierarchical_allocator_tests.cpp (line 2145)
<https://reviews.apache.org/r/41672/#comment174527>
two resources, namely cpus and mem?
Or one for each of the 2 agents allocated to this framework?
src/tests/hierarchical_allocator_tests.cpp (lines 2218 - 2219)
<https://reviews.apache.org/r/41672/#comment174530>
Why hardcode 3? I'd think you could just run a while loop and actually assert the size that you think the allocations hashmap will be. (And what key will be in each entry.)
src/tests/hierarchical_allocator_tests.cpp (lines 2223 - 2234)
<https://reviews.apache.org/r/41672/#comment174531>
Why is the resource ratio 2:1:1 when the weight ratio is 3:1:2? Seems like you might want to add more nodes to get a proper ratio?
- Adam B
On Jan. 4, 2016, 9:46 p.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2016, 9:46 p.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41672: Test case(s) for weights + allocation
behavior
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review113789
-----------------------------------------------------------
Bad patch!
Reviews applied: [41597, 41672]
Failed command: ./support/apply-review.sh -n -r 41672
Error:
2016-01-11 16:43:53 URL:https://reviews.apache.org/r/41672/diff/raw/ [7453/7453] -> "41672.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.
- Mesos ReviewBot
On Jan. 5, 2016, 5:46 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 5:46 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Test case(s) for weights + allocation behavior
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41672/diff/
>
>
> Testing
> -------
>
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN ] HierarchicalAllocatorTest.UpdateWeight
> [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [ PASSED ] 1 test.
>
>
> Thanks,
>
> Yongqiao Wang
>
>