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