impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4365: Enabling end-to-end tests on a remote cluster
Date Thu, 03 Nov 2016 21:21:16 GMT
Taras Bobrovytsky has posted comments on this change.

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

Patch Set 9:

File bin/

PS9, Line 63: 'HDFS',
            :                      'YARN',
            :                      'HIVE',
            :                      'IMPALA',
            :                      'MAPREDUCE',
            :                      'KUDU',
            :                      'HBASE',
            :                      'ZOOKEEPER
I think it would be nice to put these in sorted order. (Vim has a nice ability to sort lines

Line 174:         try:
It's kind of weird to do this through try and except. How about:

if set(REQUIRED_SERVICES) != set(setvices.keys()):

PS9, Line 177: missing_svcs
I suggest not shortening: missing_services

Line 179:             sys.exit("Cluster not ready.")
It's better to raise an exception that exiting right away. It might be unexpected that exit
is called from a method like get_services.

Line 181:         try:
it's better to do this with if then instead of assert. (same as above)

PS9, Line 253: svc_type
I prefer service_type here and elsewhere. It's clearer and easier on the brain :)

Line 379:             # 'database_name' is a header from the beeline output.
Maybe we can skip the header some other way? for example db.list.split()[1:]
File testdata/bin/

Line 482:   if [[ -n "$CM_HOST" ]]; then
this should be moved right above where it's used (line 491)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f443a1728a1d28168090c6f54e82dec2cb073e9
Gerrit-PatchSet: 9
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