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 <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
