impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
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:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4769/9/bin/remote_data_load.py
File bin/remote_data_load.py:

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
alphabetically).


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:]


http://gerrit.cloudera.org:8080/#/c/4769/9/testdata/bin/create-load-data.sh
File testdata/bin/create-load-data.sh:

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


-- 
To view, visit http://gerrit.cloudera.org:8080/4769
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f443a1728a1d28168090c6f54e82dec2cb073e9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs7@hotmail.com>
Gerrit-Reviewer: Martin Grund <grundprinzip@gmail.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message