> On 八月 25, 2016, 10:56 p.m., Jiang Yan Xu wrote: > > I don't know. Making such distinction feels like splitting hairs to me. > > Yeah there are subtle differences between the two concepts but to me: > > > > - `offerCallback` may not be the best name what we could have come up with > > even though in `Master::offer()` we do construct offers. > > - When offer objects are constructed, we call the resources in them > > "offered resources". > > - When we parse resources from offer objects, we call them "offered > > resources". > > - In this test, namely, the "Allocator" tests, we are indeed only > > allocating resources. (I don't see "Offer" objects being created). > > > > Later if the allocator actually hands out offer objects, then we probably > > need to change the tests to use offers directly but right now I think 1) > > "allocation" is most correct, 2) it's not a big deal if people treat them > > as synonymous terms. > > > > That said, I think reusing the "Allocation" defined at the top of the test > > makes sense. > > Guangya Liu wrote: > Thanks Jiang Yan, there are acutally some discussion here > https://reviews.apache.org/r/50677/ , the reason that I want to update this > is because: > > 1) Make the code consistent, in the allocator test cases, some benchmark > test using `OfferedResources` while some using `Allocation`, when people want > to add new benchmark test, they will be confused: Which one shall I use, > `OfferedResources` or `Allocation`, so here we need to make the code > consistent with each other for better reading, easy to maintain and > consistent. > > 2) Seems here using `OfferedResources` maybe better, as here we are > actually constructing the `offers` on each agent; But the `Allocation` is an > allocator concept and it means the resources for a specified framework. Also > all of the log messages are highlighting `offer` in benchmark test such as > here > https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L3885 > > 3) The benchmark test is focusing on `offers` even though the allocator > know nothing about `offer`, but here we are just simulating `offer > operations` in benchmark test. For other unit test cases other than benchmark > test, they are focusing on `allocations` for each framework, so it is using > `Allocation`. > > What do you think? > > Jiang Yan Xu wrote: > Thanks for the reply. I've listed a few critia that I think could explain > their subtle differences but I try to see if making such a distinction leads > to simplity and clarity of the code. i.e., would everyone agree with the way > we make such distinction and would it help readers understand the code > better. If it's controversial, then perhaps we should aim for consistently. :) > > In terms of code in this file, yes I agree there's inconsistency and we > should address it. To that end I think: > > To 1): The question to me is to choose between *OfferedResources -> > Allocation* or *Allocation -> OfferedResources*. I am leaning towards > *OfferedResources -> Allocation* and FWIW `Allocation` is a more established > usage than `OfferedResources` in this file. > > To 2) and 3): sorry I failed to see the fundamental difference between > the benchmarks and the unit tests here in terms of `offers` vs `allocations`. > The unit tests, espcially later-added ones, follow basically the same pattern > as the benchmarks. The benchmarks are just different in the use of > expectations vs. measurements and the number of iterations. It's not > convincing to me that in some cases we need to call them OfferedResources and > in others we should call them Allocations. > > So overall I think it's better to change to use the Allocation struct > defined at the top to achieve consistency. It looks to me that all > occurrences of `offers` in the log messages can be changed to `allocations` > as well. > > Let me know what you think. > > Guangya Liu wrote: > Hi Jiang Yan, are you mentioning the `Allocation` definition here > https://github.com/apache/mesos/blob/9c6097f063405279efc07eec22457c2059653f07/src/tests/hierarchical_allocator_tests.cpp#L79-L83 > ? They are a bit different: > > ``` > struct Allocation > { > FrameworkID frameworkId; > hashmap<SlaveID, Resources> resources; > }; > ``` > > ``` > struct Allocation > { > FrameworkID frameworkId; > SlaveID slaveId; > Resources resources; > }; > ``` > > The above two is a bit difference: The first `Allocation` is an > allocation for a framework and the second `Allocation` is actullay an `offer` > as one framework can have multiple such `Allocation`. > > So it may not accurate to use `allocations` in the benchmark test as they > are actually `offers` for each framework, comments? Thanks. > > Jiang Yan Xu wrote: > Yes > [this](https://github.com/apache/mesos/blob/9c6097f063405279efc07eec22457c2059653f07/src/tests/hierarchical_allocator_tests.cpp#L79-L83) > is what I mean. > > Why is it "actullay an offer"? The way I look at it, the frameworks get > allocations from the master and they just recover them in this test. > > If the Allocation is defined as > > ``` > struct Allocation > { > FrameworkID frameworkId; > hashmap<SlaveID, Resources> resources; > }; > > ``` > > Then we do this to recover it: > > ``` > foreach (const Allocation& allocation, allocations) { > foreachpair (const SlaveID& slaveId, > const Resources& resources, > allocation) { > allocator->recoverResources( > allocation.frameworkId, > slaveId, > resources, > None()); > } > ``` > > It's the same isn't it?
Yes, that make sense, then we will use `allocations` in the benchmark test. will update this later. Thanks @jiang Yan. - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50868/#review146878 ----------------------------------------------------------- On 八月 5, 2016, 9:50 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50868/ > ----------------------------------------------------------- > > (Updated 八月 5, 2016, 9:50 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > The current benchmark test of `SuppressOffers` is using `Allocation` > to specify the resources from an `OfferCallback`, but here seems > using `OfferedResources` maybe better, as here we are actually > constructing the offers on each agent, but the `Allocation` is an > allocator concept and it means the resources allocated for a specified > framework. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > cbed333f497016fe2811f755028796012b41db77 > > Diff: https://reviews.apache.org/r/50868/diff/ > > > Testing > ------- > > make > make check > > ``` > ./bin/mesos-tests.sh --benchmark > --gtest_filter="*HierarchicalAllocator_BENCHMARK_Test.SuppressOffers/0" > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test > [ RUN ] > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.SuppressOffers/0 > Using 1000 agents and 1 frameworks > Added 1 frameworks in 841us > Added 1000 agents in 480991us > allocate() took 264196us to make 1000 offers with 0 out of 1 frameworks > suppressing offers > allocate() took 270110us to make 1000 offers with 0 out of 1 frameworks > suppressing offers > allocate() took 270098us to make 1000 offers with 0 out of 1 frameworks > suppressing offers > allocate() took 264314us to make 1000 offers with 0 out of 1 frameworks > suppressing offers > allocate() took 268732us to make 1000 offers with 0 out of 1 frameworks > suppressing offers > [ OK ] > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.SuppressOffers/0 > (4511 ms) > [----------] 1 test from > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (4511 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (4522 ms total) > [ PASSED ] 1 test. > ``` > > > Thanks, > > Guangya Liu > >
