impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5164: Fix flaky benchmarks
Date Mon, 22 May 2017 19:23:01 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5164: Fix flaky benchmarks
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6935/2/be/src/util/benchmark.cc
File be/src/util/benchmark.cc:

Line 35: double Benchmark::Measure(BenchmarkFunction function, void* args,
> Can we change the behaviour so that benchmarks can opt in or out from the c
Definitely.  I noticed a few tests with more complicated behavior that definitely don't want
this (unfortunately they crash instead of running).


Line 62:       throw std::runtime_error("Benchmark failed to complete due to context switching.");
> We usually use LOG(FATAL) for fatal errors or Status for non-fatal errors. 
That is true, but having an error message and call graph doesn't help here.  Instead, I want
to throw an error which can be caught and logged at the level where we have context on which
benchmark was being run (see the sample error message in the summary).


Line 69:   }
> Maybe we should reset 'sw' and 'lost_time' after doing the tuning step so w
I thought so as well, but there is no reset method currently, so this actually seems easiest.


PS2, Line 94: 10
> Where does the 10 come from?
Arbitrary flake factor.  If we lose 90% of measurements, we bail.  This parameter just made
the arithmetic easy.


Line 95:     throw std::runtime_error("Benchmark failed to complete due to noisy measurements.");
> Same comment here about using Status/LOG(FATAL).
Same reason - we want context on which benchmark was failing, which we don't have here.


-- 
To view, visit http://gerrit.cloudera.org:8080/6935
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7e0ddfaa9bd1340990ae2b6ff946d72bbd26c274
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message