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 <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
