impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4365: Enabling end-to-end tests on a remote cluster
Date Tue, 01 Nov 2016 22:08:31 GMT
David Knupp has posted comments on this change.

Change subject: IMPALA-4365: Enabling end-to-end tests on a remote cluster

Patch Set 7:

File bin/

PS7, Line 17: This script is a setup script
> How about simply "this is a setup script"

PS7, Line 21: The script expects a setup Impala development environment.
> How about:

PS7, Line 22: appropriately
> It's not clear what this means. Maybe expand on this a little?

PS7, Line 47: import logging
> import statements should be above from x import y

PS7, Line 111: ,
> remove

PS7, Line 116: that,
> remove

PS7, Line 127: not
> having a not here makes it harder to reason about, how about:

PS7, Line 130: password=options.cm_pass)
> Is this indent correct according to the style guide? I thought it was suppo
This passes PEP-8 linter inspection.

PS7, Line 204: Only pick the first cluster configured
> This comment does not really add anything, it's obvious what the code is do

PS7, Line 209: for service in cluster.get_all_services():
> Just an observation: it looks like we don't check that all services we need
I think that confirming the readiness of the cluster is a good idea. I'm adding a method to
do this as part of the init procedure. Right now, all it does is check that the required services
are installed and running. It can be extended later to do other checks as well.

Line 281:         # See
> It looks like the issue is resolved, is this still necessary?

PS7, Line 338: "database_name"
> Why would it ever be called "database_name"? Where is this constant coming 
I figured out it's the header from the beeline output. (Much of this code, I've inherited,
and I'll confess I didn't catch every line.) I'll add a comment.

PS7, Line 432: wmay
> may

PS7, Line 451: # "--skip_local_tests",
> why is this commented out?
Bascially, this is another remainder from Martin was last working on the script. In fact,
this entire method may go away (it's not currently working to use runtest to run remote cluster
tests, but you can do it calilng impala-py.test directly.)
File testdata/bin/

Line 492:     run-step "Loading nested data" load-nested.log \
> The command is very similar in both cases. The only difference is --cm-host
File testdata/bin/

Line 411:     if os.getenv("REMOTE_LOAD"):
> base_load_dir = os.getenv("REMOTE_LOAD") or os.getenv("IMPALA_HOME")
File testdata/bin/

Line 301:   parser.add_argument("-s", "--source-db", default="tpch_parquet")
> don't we also need cm-host parameter here?
Yup, but we pick that up from L300:


Good eye though. I'll add a comment.
File testdata/bin/

Line 20: set -e
> we are already setting -euo pipefail below

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f443a1728a1d28168090c6f54e82dec2cb073e9
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Harrison Sheinblatt <>
Gerrit-Reviewer: Martin Grund <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message