impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests
Date Fri, 17 Feb 2017 21:35:32 GMT
Michael Brown has posted comments on this change.

Change subject: IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive

Patch Set 4: Code-Review+1


Thanks for the review, Jim. Patch set 4 does not have any functional changes relative to patch
set 3. Some of your comments I answered by making changes to the comments in code; most I
answer as comments in Gerrit.

Carry +1
Commit Message:

Line 7: IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests
> long line
Can you please suggest a meaningful header that summarizes the patch that also fits in 72
columns? "Long line" on its own isn't helpful. I realize it's long, but I'm not sure how to
shorten it without loss of information.

Line 29: An exhaustive build+test run showed test_ddl_stress and TestSpillStress
> Roughly how much longer does this make exhaustive runs?
File bin/

PS3, Line 100: XXX
> For my education: what do the three X's mean
Vim highlights it (like how it highlights "TODO"). I like to use them as notices.

PS3, Line 102: `
> I don't think I've seen this idiom before. I recognize it is in HEAD alread
That's the gist. I expect it's either a Martinism or Caseyism. I like it: it allows for clearer
formatting and organization of multi-line quoted text on RHS of an assignment. Note that parentheses
won't work, nor will backslash.

Line 188:            "its dependencies. If already running, all services are restarted."\
> What happened to the spaces?
I noticed this when running -help They are not needed and ./ -help will
show two spaces (on master). See this gist for more info, if you care. :) I don't want to
clutter the review with the explanation.

PS3, Line 196: ONLY APPLIES to suites with workloads:"\
             :            "functional-query, targeted-stress"
> This seems likely to become stale if that changes, as it may not show up to
I think it's important to convey it *somewhere* what "running exhaustively" actually means. is the common entry point to our system, and we already have help text here. I
simply clarified the help text. I also left a notice (that "XXX" you asked about) in hopes
people will be prompted to update it.
File tests/custom_cluster/

PS3, Line 56: ) and ca
> Would it be worthwhile or even possible to fix it to be more typical? If so

PS3, Line 66: equivalent
> This one I don't quite understand. The CustomClusterTestSuite presumably sh
File tests/stress/

Line 59
> Does removing this (to pytest.mark.parametrize) change the possible orderin
No change in ordering of DDL calls. The CREATE DATABASE that is executed now is part of the
CREATE DATABASE from the unique_database fixture before the test proper starts. The difference
is each test will use its own unique database instead of sharing across "ddl_stres_testdb".

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message