mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 50868: Using `OfferedResources` for benchmark test of `SuppressOffers`.
Date Mon, 29 Aug 2016 17:26:15 GMT


> On Aug. 25, 2016, 3: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?

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.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50868/#review146878
-----------------------------------------------------------


On Aug. 5, 2016, 2:50 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50868/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2016, 2: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message