Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Alexander Rukletsov]

> On Aug. 6, 2018, 11:40 a.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Lines 628 (patched)
> > 
> >
> > The `async()` seems unnecessary here, we can just call the function 
> > directly.

This is true. However, it does not hurt and _looks_ more consistent, hence I'd 
leave it unless you have a strong opinion.


> On Aug. 6, 2018, 11:40 a.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Lines 634 (patched)
> > 
> >
> > Maybe we should add one run here where we query only `stateEndpoint` 
> > without hitting `indicatorEndpoint` at the same time?

I don't think we should. The aim of the test is to measure the responsiveness 
of the master actor and not the performance of the '/state' endpoint.


- Alexander


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


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Alexander Rukletsov]

> On Aug. 8, 2018, 1:19 a.m., Benjamin Mahler wrote:
> > Just a high level comment, it's unfortunate that this test depends so much 
> > on timing w.r.t. to the 200ms polling interval. For example, if we actually 
> > serve the first request in less than 200ms then we wouldn't be measuring 
> > the benefits of batching.
> > 
> > An idea for how to fix this:
> > 
> > * Issue batches, e.g. N requests at once, when they all finish, another N 
> > requests, only need to repeat a few times. This means that the benchmark 
> > will always show how batching can help high load, and we're not assuming 
> > requests take longer than e.g. 200ms to get processed on the master. We 
> > don't need an interval here because we can just proceed with the next poll 
> > as soon as everything finished from the first one.

I think you suggestion is biased towards the batching approach: if we send 
request in batches then obviously processing them together is the best 
strategy. However, if no two '/state' requests sit in the master mailbox at any 
time, then batching is strictly worse. The reason why we decided to do batching 
is because:
1. The negative effect of batching in the second scenario is much smaller than 
the positive effect in the first scenario. The second scenario implies that the 
master mailbox is relatively empty hence an extra trip is negligible.
2. The main goal is to isolate the master workload from state-related 
computations and **not** speed up '/state' responses. Hence the important 
metric here is '/flags' response time.

I would like to keep the test not opinionated and have it mimic the request 
rate we saw in real world clusters.


- Alexander


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


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Benjamin Mahler]

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



Just a high level comment, it's unfortunate that this test depends so much on 
timing w.r.t. to the 200ms polling interval. For example, if we actually serve 
the first request in less than 200ms then we wouldn't be measuring the benefits 
of batching.

An idea for how to fix this:

* Issue batches, e.g. N requests at once, when they all finish, another N 
requests, only need to repeat a few times. This means that the benchmark will 
always show how batching can help high load, and we're not assuming requests 
take longer than e.g. 200ms to get processed on the master. We don't need an 
interval here because we can just proceed with the next poll as soon as 
everything finished from the first one.


src/tests/master_benchmarks.cpp
Lines 512 (patched)


The review summary needs to updated to reflect the renaming?


- Benjamin Mahler


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Benno Evers]

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


Fix it, then Ship it!





src/tests/master_benchmarks.cpp
Lines 496 (patched)


Is it possible to store the last parameter directly as 
`process::Milliseconds`?



src/tests/master_benchmarks.cpp
Lines 628 (patched)


The `async()` seems unnecessary here, we can just call the function 
directly.



src/tests/master_benchmarks.cpp
Lines 634 (patched)


Maybe we should add one run here where we query only `stateEndpoint` 
without hitting `indicatorEndpoint` at the same time?


- Benno Evers


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Alexander Rukletsov]

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

(Updated Aug. 6, 2018, 10:30 a.m.)


Review request for mesos, Benno Evers and Benjamin Mahler.


Bugs: MESOS-8975
https://issues.apache.org/jira/browse/MESOS-8975


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 


Diff: https://reviews.apache.org/r/68131/diff/2/

Changes: https://reviews.apache.org/r/68131/diff/1-2/


Testing
---

See https://reviews.apache.org/r/68132/


Thanks,

Alexander Rukletsov