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 50693: Added another function `createSlaveInfo` for allocator benchmark test.
Date Fri, 02 Sep 2016 21:06:38 GMT


> On Sept. 1, 2016, 1:50 p.m., Jiang Yan Xu wrote:
> > The rationale isn't clear to me: for the same test if we always parse the resources
at the same place, isn't the measurement still accurate when you compare the benchmark results
with vs. without certain patches or between releases? The absolute numbers don't matter too
much as they are not indicative of "real" workloads anyways right?
> > 
> > I am OK with adding this method though and Anindya has another patch https://reviews.apache.org/r/49571/
that does this because we need to construct more complex resources. Will it be OK to just
have this method in https://reviews.apache.org/r/49571/?
> 
> Guangya Liu wrote:
>     Thanks Jiang Yan, what about make the patch of https://reviews.apache.org/r/49571/
focus on shared resource benchmark test while this one focus on the new API for `createSlaveInfo`?
The `createSlaveInfo` will involve many changes and handling this in a separate patch maybe
better?
> 
> Jiang Yan Xu wrote:
>     By "will involve many changes" do you mean you'd like to change many/all call-sites
to use the new method? I am just saying I don't see why we need to change the existing call-sites
for "benchmark accuracy concerns". 
>     
>     How about in this patch we just add this method itself?
> 
> Guangya Liu wrote:
>     The reason is that I do not want to let the benchmark test to call old `createSlaveInfo`
as this will call `Resources::parse` each time when adding a new agent. So I was only updating
the caller part for the `benchmark` test, make sense?

To clarify, I am fine with adding this method; I am neutral with changing the benchmarks to
use it (hence ship it). I am just not sure about the argument about the "accuracy" of benchmarks.
I have stated my reasoning in the comments above. What's you view on this? Could you adjust
the review summary?

i.e., I would just say 

```
Summary: Added a `createSlaveInfo()` overload that takes a `Resources`.

Description: Also changed the benchmarks to use it because it's unnecessary to recreate the
same Resources objects for each agent.
```


- Jiang Yan


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


On Sept. 2, 2016, 4:22 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2016, 4:22 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In allocator benchmark test, when `addSlave`, the test will first
> create slave info, but currently, the parameter for `createSlaveInfo`
> is a resource string, and this caused the `createSlaveInfo` will
> always parse resource first before set resource for the agent. This
> caused the time of adding agent is not accurate.
> 
> This fix is adding another function named as `createSlaveInfo` but
> taking `Resources` as input parameter, this will remove the time
> of parsing resources when setting resource for the agent and thus
> making the time of adding agent more accurate.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp d960b7575ed5531753e9329e5774b6909090edf8

> 
> Diff: https://reviews.apache.org/r/50693/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> Before fix adding 30000 agents.
> ```
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN      ] SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1216us
> Added 1 frameworks in 509us
> Added 30000 agents in 14.515326secs
> /metrics/snapshot took 48615us for 30000 agents and 1 frameworks
> [       OK ] SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14679
ms)
> [----------] 1 test from SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
(14679 ms total)
> ```
> 
> After fix adding 30000 agents.
> ```
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN      ] SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1238us
> Added 1 frameworks in 555us
> Added 30000 agents in 13.976131secs
> /metrics/snapshot took 58360us for 30000 agents and 1 frameworks
> [       OK ] SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14139
ms)
> [----------] 1 test from SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
(14140 ms total)
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


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