You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/02/19 10:31:40 UTC
Re: Review Request 41672: Test case(s) for weights + allocation
behaviour.
-----------------------------------------------------------
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 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
>
>