Michael Brown has posted comments on this change. Change subject: Add a script to test performance on a developer machine ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/6818/4/bin/single_node_perf_run.py File bin/single_node_perf_run.py: PS4, Line 110: git_hash = sh.git("rev-parse", "HEAD").strip() get_git_hash_for_name("HEAD") ? PS4, Line 254: parser.add_option("--no_load", dest="load", action="store_false", default=True, : help="use already-present workload databases") : parser.add_option("--no_start_minicluster", dest="start_minicluster", : action="store_false", default=True, : help="use already-running minicluster") It's confusing to me to have --no options that default to True that are stored_false. I can't keep track of what's going on and what the defaults will actually be. Are there ways you could make it less confusing? Can you reduce some of the negativity? PS4, Line 262: parser.set_usage("single_node_perf_run.py [options] git_hash_A [git_hash_B]\n\n" : "Compares the performance of Impala at two git hashes on\n" : "some standard benchmarks. Output is in\n" : "$IMPALA_HOME/perf_results/latest.\n\n" : "WARNING: This script will run git checkout. You should not touch\n" : "the tree while the script is running. You should start the script\n" : "from a clean git tree.\n\n" : "WARNING: Unless --no_load is used, this script calls load_data.py\n" : "which can overwrite your TPC-H and TPC-DS data.") Nit: Consider a triple-quoted string and using textwrap.dedent(). parser.set_usage(textwrap.dedent("""\ single_node_perf_run.py [options] git_hash_A [git_hash_B] Compares the performance of Impala [etc] """) PS4, Line 296: main() 2 spaces. -- 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: 4 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
