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 <dkn...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Harrison Sheinblatt <h...@hotmail.com> Gerrit-Reviewer: Martin Grund <grundprin...@gmail.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
