Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201705232158.v4NLwY6p011671@ip-10-146-233-104.ec2.internal>
Date: Tue, 23 May 2017 21:58:34 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Zach Amsden <zamsden@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5164=3A_Fix_flaky_benchmarks=0A?=
In-Reply-To: <gerrit.1495242893000.I7e0ddfaa9bd1340990ae2b6ff946d72bbd26c274@gerrit.cloudera.org>
References: <gerrit.1495242893000.I7e0ddfaa9bd1340990ae2b6ff946d72bbd26c274@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Tue, 23 May 2017 21:58:39 -0000

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