impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
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
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 <mikeb@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message