[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

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


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6002/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for 
exhaustive tests
> Although I'm not keen on the strict 72-char limit (we have many other commi
Thanks for the point Henry. I once saw on an Impala code review (I can't find 
it to cite it) a fair refute of this idea: writing Git commit headers like 
"IMPALA-{4904, 4914}" was a bad idea, because if someone needs to search Git 
logs for IMPALA-4914, he won't match this particular header.


-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

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


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6002/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for 
exhaustive tests
> I agree this loses some information, but I think it's the right tradeoff:
Although I'm not keen on the strict 72-char limit (we have many other commit 
messages that go over this - what's the reason for enforcing it?) - note you 
can save some chars by compressing the list of fixed JIRAs:

  IMPALA-{4904, 4914}: etc.


-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

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


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6002/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for 
exhaustive tests
> Can you please suggest a meaningful header that summarizes the patch that a
I agree this loses some information, but I think it's the right tradeoff:

IMPALA-4904,IMPALA-4914: add targeted-stress to exhaustive tests


http://gerrit.cloudera.org:8080/#/c/6002/3/tests/stress/test_ddl_stress.py
File tests/stress/test_ddl_stress.py:

Line 59
> No change in ordering of DDL calls. The CREATE DATABASE that is executed no
Given the number of catalog race conditions we tend to see, I think it is 
possible that this changes the test meaning.

My feeling is that it would be good to test both things: multiple tables in one 
DB and multiple tables each with its own DB.

If this method and TEST_IDS were left unchanged, would the test fail?


-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
Gerrit-PatchSet: 3
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

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

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


Patch Set 4: Code-Review+1

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/6002/3//COMMIT_MSG
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?
Done


http://gerrit.cloudera.org:8080/#/c/6002/3/bin/run-all-tests.sh
File bin/run-all-tests.sh:

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.


http://gerrit.cloudera.org:8080/#/c/6002/3/buildall.sh
File buildall.sh:

Line 188:"its dependencies. If already running, all services are 
restarted."\
> What happened to the spaces?
I noticed this when running buildall.sh -help They are not needed and 
./buidall.sh -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.

https://gist.github.com/mikesbrown/ca0c5e4a2ba13dd929f72c57adf4e8bc


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. buildall.sh 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.


http://gerrit.cloudera.org:8080/#/c/6002/3/tests/custom_cluster/test_spilling.py
File tests/custom_cluster/test_spilling.py:

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


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


http://gerrit.cloudera.org:8080/#/c/6002/3/tests/stress/test_ddl_stress.py
File tests/stress/test_ddl_stress.py:

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 http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[Michael Brown (Code Review)]

Hello David Knupp,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6002

to look at the new patch set (#4).

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

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

This patch allows any test suites whose workload is "targeted-stress" to
be run in so-called "exhaustive" mode. Before this patch, only suites
whose workload was "functional-query" would be run exhaustively. More on
this flaw is in IMPALA-3947.

The net effects are:

1. We fix IMPALA-4904, which allows test_ddl_stress to start running
   again.
2. We also improve the situation in IMPALA-4914 by getting
   TestSpillStress to run, though we don't fix its
   not-running-concurrently problem.

The other mini-cluster stress tests were disabled in this commit:
  IMPALA-2605: Omit the sort and mini stress tests
so they are not directly affected here.

I also tried to clarify what "exhaustive" means in some of our shell
scripts, via help text and comments.

An exhaustive build+test run showed test_ddl_stress and TestSpillStress
now get run and passed. This adds roughly 12 minutes to a build that's
on the order of 13-14 hours.

Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
---
M bin/run-all-tests.sh
M buildall.sh
M tests/custom_cluster/test_spilling.py
M tests/stress/test_ddl_stress.py
4 files changed, 67 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/6002/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
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 

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

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


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6002/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for 
exhaustive tests
long line


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


http://gerrit.cloudera.org:8080/#/c/6002/3/bin/run-all-tests.sh
File bin/run-all-tests.sh:

PS3, Line 100:   `
For my education: what do the three X's mean


PS3, Line 102: 
I don't think I've seen this idiom before. I recognize it is in HEAD already, 
but, for the purposes of my own education, is this command substitution where 
the command contains only whitespace, thereby just acting like "remove these 
backticks and all the space between them"?


http://gerrit.cloudera.org:8080/#/c/6002/3/buildall.sh
File buildall.sh:

Line 188:"its dependencies. If already running, all services are 
restarted."\
What happened to the spaces?


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 the 
author of a change to run-all-tests.sh. Is there anything to be done about that?


http://gerrit.cloudera.org:8080/#/c/6002/3/tests/custom_cluster/test_spilling.py
File tests/custom_cluster/test_spilling.py:

PS3, Line 56: 
Would it be worthwhile or even possible to fix it to be more typical? If so, 
should that be this patch or should it be a new JIRA?


PS3, Line 66: 
This one I don't quite understand. The CustomClusterTestSuite presumably shuts 
down impalad, but that's OK once this test is done, yes?


http://gerrit.cloudera.org:8080/#/c/6002/3/tests/stress/test_ddl_stress.py
File tests/stress/test_ddl_stress.py:

Line 59
Does removing this (to pytest.mark.parametrize) change the possible orderings 
of the DDL calls?


-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
Gerrit-PatchSet: 2
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

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

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


Patch Set 3: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6bd5bbd380e636d680368e908519b042d79dfec
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4904,IMPALA-4914: whitelist workload "targeted-stress" for exhaustive tests

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

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


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6002/1/bin/run-all-tests.sh
File bin/run-all-tests.sh:

PS1, Line 95:  around in many formats.
Nit. I had to read this sentence a couple of times. How do you feel about this 
wording?

"too large to load in all of the formats required by exhaustive mode."

Or something like that.


http://gerrit.cloudera.org:8080/#/c/6002/2/tests/custom_cluster/test_spilling.py
File tests/custom_cluster/test_spilling.py:

PS2, Line 57: super(CustomClusterTestSuite, cls).setup_class()
What's the reason to not have this at the very top of the method?


-- 
To view, visit http://gerrit.cloudera.org:8080/6002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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