Michael Brown has posted comments on this change. Change subject: Add a script to test performance on a developer machine ......................................................................
Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/6818/2//COMMIT_MSG Commit Message: PS2, Line 12: bin/jenkins/ Remove jenkins. http://gerrit.cloudera.org:8080/#/c/6818/2/bin/single_node_perf_run.py File bin/single_node_perf_run.py: Line 22: # Compares the performance of Impala at two git hashes on It might be good to include a large, prominent note saying that this will check out the hashes and alter your checkout. Line 23: # some standard benchmarks. Output is in It might also be good to call out load_data.py will get called and could be destructive when re-creating databases and tables. PS2, Line 59: impala_home = os.environ["IMPALA_HOME"] Please make IMPALA_HOME all-caps, as that's a convention for global variables. PS2, Line 125: result = "{0}/performance_result.txt".format(impala_home) Do you not want to write this file into the perf_results directory? PS2, Line 183: sh.cp(os.path.join(source, "workloads"), os.path.join(impala_home, "testdata"), : R=True, _out=sys.stdout, _err=sys.stderr) What happens if one of these sh.cp() commands fail? Will the script completely halt, or will it just fail silently, or...? PS2, Line 264: raise Exception("Invalid number of arguments") Maybe a clearer message? "Invalid arguments: either 1 or 2 GIt hashes allowed" (or something to that effect). You could also call parser.print_help() and exit 1 as opposed to raising an exception. PS2, Line 268: raise Exception("Can't run when a commit is needed") Maybe: "Working copy is dirty. Consider 'git stash' and try again." Also, what happens if there are changes to the index? "git diff" will not catch those. PS2, Line 271: current_hash = sh.git("rev-parse", "--abbrev-ref", "HEAD").strip() You probably want to chdir(impala_home) first? http://gerrit.cloudera.org:8080/#/c/6818/2/testdata/datasets/tpcds/preload File testdata/datasets/tpcds/preload: PS2, Line 52: if [ -t 1 ] Cool. Did you test both? -- To view, visit http://gerrit.cloudera.org:8080/6818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I70ba7f3c28f612a370915615600bf8dcebcedbc9 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
