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 Tue, 23 May 2017 01:05:17 GMT
Zach Amsden has posted comments on this change.

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


Patch Set 2:

(4 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.");
> Sounds like a Status might be a better fit then.
Making this return a status means we have to return iters / ms as a reference parameter or
return via pointer.  This spoils the convention that such return parameters should be the
last parameters in the functions parameter list because in this case we have default parameters
that are overridden almost nowhere.

Not a big fan of exceptions, especially across modules, but this self contained single source
file use case might be the lesser of two evils.

Happy to do it either way.


Line 69:   }
> You could maybe create a new stopwatch in that case?
Doing so requires throwing away the results of the first set of computations or more complicated
math on the return value.  Neither seems especially better than what the code does currently.


Line 90:       batch_size = max<int>(1, batch_size / (1+(ru_stop.ru_nivcsw - ru_start.ru_nivcsw)));
> Maybe comment why we're scaling it down here?
Done


PS2, Line 94: 10
> Can you add a comment along those lines for future reference then?
Done


-- 
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