Tim Armstrong has posted comments on this change. Change subject: IMPALA-5164: Fix flaky benchmarks ......................................................................
Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/6935/2/be/src/util/benchmark.cc File be/src/util/benchmark.cc: Line 62: throw std::runtime_error("Benchmark failed to complete due to context switching."); > Making this return a status means we have to return iters / ms as a referen Yeah I've run into that optional param/return param thing before, it's annoying. I guess the exception is OK if we add some explanation to the method comment (i.e. the intent is to crash the process when the benchmark doesn't behave as expected). We could also consider exposing two interfaces: the current one, which does a LOG(FATAL) if there's an error, and an internal "advanced" one without default arguments that returns a Status. Line 69: } > Doing so requires throwing away the results of the first set of computation Sounds ok -- 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 <zams...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
