> On 七月 14, 2016, 5:56 p.m., Benjamin Mahler wrote: > > include/mesos/resources.hpp, lines 135-147 > > <https://reviews.apache.org/r/50017/diff/1/?file=1443931#file1443931line135> > > > > This suggests a change of semantics to `Resources`. > > > > Currently: `Resources` always refers to a valid collection resources. > > This function would never return Some(Error) unless we change the semantics. > > > > If we want to remove the unnecessary validation, we can do this by > > making '`operator += (const Resources& that)`' avoid validating '`that`' > > since it's already valid. Currently it just calls '`operator += (const > > Resource& r)`' which is unaware of '`r`' already being valid since it comes > > from a '`Resources`'. > > > > However, we have to keep the validation in '`operator += (const > > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary > > caller, it may be invalid and we need to validate. > > > > Make sense? > > Guangya Liu wrote: > Hi Ben, does there are any example in mesos code for `Resource` object > come from protobuf? I found that we are always using `Resource::parse` to get > a `Resource` or `Resources` object and I have added some validation logic in > `Resource::parse` in when getting `Resource` or `Resources` object.
Ben, got your point. We can set `Resource` object as following: ```javascript Resource scalar; scalar.set_name("cpus"); scalar.set_type(Value::SCALAR); scalar.mutable_scalar()->set_value(1.234); ``` So if we igore the validation for `'operator += (const Resource& r)'`, there might be some erros if the resource was not constructed correctly. This is not a good way to create `Resource` object but we cannot guard the usage for this now. I found that the API `'operator += (const Resource& r)'` was not called directly too frequently, but we are using this API `'operator += (const Resources& that)'` for the most of the time, so seems it is good to keep the validation for `'operator += (const Resource& r)'` as it was not called directly too much. The problem is that `'operator += (const Resources& that)'` is calling `'operator += (const Resource& r)'`, I did not found a good way to keep validation for `'operator += (const Resource& r)'` but ignore the validation for `'operator += (const Resources& that)'`, still checking how we can make this work, any comments/suggestions for this? - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50017/#review142257 ----------------------------------------------------------- On 七月 14, 2016, 3:59 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50017/ > ----------------------------------------------------------- > > (Updated 七月 14, 2016, 3:59 p.m.) > > > Review request for mesos, Benjamin Mahler and Klaus Ma. > > > Repository: mesos > > > Description > ------- > > The "validation" API was called in a huge number of times, but this > was only needed when parsing resources and we do not need to do > other validation when doing resources operations, such as add etc. > > > Diffs > ----- > > include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 > include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 > src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 > src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a > > Diff: https://reviews.apache.org/r/50017/diff/ > > > Testing > ------- > > The `qcachegrind` result here: > https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing > > After the fix, the performance of `adding` resources increased 30+% when > adding 50000 agents to the cluster. > > Before fix: > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 > Using 50000 agents and 1 clients > Added 1 clients in 47us > **Added 50000 agents in 1.312497secs** > Sorted 1 clients in 43us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (1321 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 > Using 50000 agents and 50 clients > Added 50 clients in 948us > **Added 50000 agents in 1.325987secs** > Sorted 50 clients in 1165us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 (1340 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 > Using 50000 agents and 100 clients > Added 100 clients in 1697us > **Added 50000 agents in 1.409478secs** > Sorted 100 clients in 2876us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 (1432 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 > Using 50000 agents and 200 clients > Added 200 clients in 4553us > **Added 50000 agents in 1.371473secs** > Sorted 200 clients in 5371us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 (1412 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 > Using 50000 agents and 500 clients > Added 500 clients in 8836us > **Added 50000 agents in 1.304245secs** > Sorted 500 clients in 14697us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 (1387 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 > Using 50000 agents and 1000 clients > Added 1000 clients in 19508us > **Added 50000 agents in 1.270555secs** > Sorted 1000 clients in 32575us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (1433 ms) > > > After the fix: > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 > Using 50000 agents and 1 clients > Added 1 clients in 42us > **Added 50000 agents in 891266us** > Sorted 1 clients in 59us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (902 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 > Using 50000 agents and 50 clients > Added 50 clients in 933us > **Added 50000 agents in 885006us** > Sorted 50 clients in 1220us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 (899 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 > Using 50000 agents and 100 clients > Added 100 clients in 1879us > **Added 50000 agents in 903112us** > Sorted 100 clients in 2800us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 (922 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 > Using 50000 agents and 200 clients > Added 200 clients in 3893us > **Added 50000 agents in 881240us** > Sorted 200 clients in 5802us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 (912 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 > Using 50000 agents and 500 clients > Added 500 clients in 10712us > **Added 50000 agents in 877442us** > Sorted 500 clients in 17887us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 (949 ms) > [ RUN ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 > Using 50000 agents and 1000 clients > Added 1000 clients in 21472us > **Added 50000 agents in 916653us** > Sorted 1000 clients in 37369us > [ OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (1057 ms) > > > Thanks, > > Guangya Liu > >