impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5164: Fix flaky benchmarks
Date Tue, 23 May 2017 21:58:34 GMT
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 <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