mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.
Date Tue, 06 Nov 2018 12:19:32 GMT

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


Fix it, then Ship it!





src/tests/master_benchmarks.cpp
Line 493 (original), 503 (patched)
<https://reviews.apache.org/r/68131/#comment295016>

    Do we still need the `Multiclient` suffix if we're not going to commit the `Delay` version?



src/tests/master_benchmarks.cpp
Line 598 (original), 605 (patched)
<https://reviews.apache.org/r/68131/#comment295007>

    As far as I can tell, we only use `ATOMIC_VAR_INIT` in libprocess, in Mesos the usual
constructor is used. (see e.g. the atomic `offerCallbacks` in `hierarchical_allocator_tests.cpp`)
    
    Also, per our style guide `std::atomic_bool` is preferred over `std::atomic<bool>`.



src/tests/master_benchmarks.cpp
Line 600 (original), 607 (patched)
<https://reviews.apache.org/r/68131/#comment295008>

    s/exist/exit/



src/tests/master_benchmarks.cpp
Line 604 (original), 611 (patched)
<https://reviews.apache.org/r/68131/#comment295010>

    %s/repeateRequests/repeatedRequests/



src/tests/master_benchmarks.cpp
Lines 616 (patched)
<https://reviews.apache.org/r/68131/#comment295013>

    Last time we tried running this benchmark, we discovered a dead-lock caused by the interaction
between the use of libprocess worker threads in both our test code and in the mesos code.

    
    Ideally we wouldn't use libprocess at all here, but to avoid another big change late in
the review cycle, adding a check like this should be sufficient:
    ```
    if (numClients >= LIBPROCESS_NUM_WORKER_THREADS - 3) {
      cerr << "Not enough worker threads for the desired number of clients.";
      exit(1);
    }
    ```



src/tests/master_benchmarks.cpp
Lines 663 (patched)
<https://reviews.apache.org/r/68131/#comment295009>

    You should probably check that the future wasn't failed before calling `.get()`.



src/tests/master_benchmarks.cpp
Line 640 (original), 674-677 (patched)
<https://reviews.apache.org/r/68131/#comment295014>

    I'm a bit confused by the intention behind the stop condition:
    
    My best guess is that you want to ensure that you have constant load during the whole
duration of the benchmark. However, in that case it seems like the requests to `/state` should
not be limited to a specific number but continue indefinitely, and `stop` should be set to
true after the required number of requests to the indicator endpoint have happened.
    
    On the other hand, if the current implementation is as intended, I think the message should
read `launching *up to* {numRequests} requests`, because there's no guarantee that the loop
with requests for the indicator endpoint will be the one finishing first. Also, in this case
I think the comments could be a bit more specific about the intention.



src/tests/master_benchmarks.cpp
Lines 693 (patched)
<https://reviews.apache.org/r/68131/#comment295012>

    I would advise against using `auto` here, since figuring out the actual type is not trivial.
(one technically has to go the declaration of `process::collect()` in a different file)



src/tests/master_benchmarks.cpp
Line 646 (original), 702 (patched)
<https://reviews.apache.org/r/68131/#comment295015>

    Given that we're capturing and printing baseline statistics anyways, would it make sense
to print the relative slowdown between baseline performance and performance under load here,
to get a metric that is comparable across Mesos versions?


- Benno Evers


On Nov. 4, 2018, 4:31 a.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2018, 4:31 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/4/
> 
> 
> Testing
> -------
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


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