> 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 > >
