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 Fri, 04 Nov 2016 00:44:20 GMT
David Knupp has posted comments on this change.

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

Patch Set 9:


Addressed comments. Also removed another place in where sys.exit() was
called. Finally, fixed a bug that I noticed in the arg parsing code.
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 abil

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

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 unex

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 br

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