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