impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-ASF-CR] Add a script to test performance on a developer machine
Date Mon, 22 May 2017 15:48:57 GMT
Michael Brown has posted comments on this change.

Change subject: Add a script to test performance on a developer machine

Patch Set 4:

File bin/

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(" [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\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("""\ [options] git_hash_A [git_hash_B]

    Compares the performance of Impala [etc]

PS4, Line 296:     main()
2 spaces.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I70ba7f3c28f612a370915615600bf8dcebcedbc9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-HasComments: Yes

View raw message