David Knupp has posted comments on this change. Change subject: IMPALA-1671: Print time and link to coordinator web UI once query is submitted in shell ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/3507/2/shell/impala_shell.py File shell/impala_shell.py: PS2, Line 676: if version Curious -- is there ever a time when we'd get successfully connect here (i.e., not trip one of the following exceptions), and yet still get back a falsey value from connect()? PS2, Line 858: try This try block is getting quite long. Long try blocks followed by several except clauses are sometimes considered a code smell. I wonder if maybe this should be broken up such that individual sections of code are wrapped inside try/except blocks that catch only the relevant error to that code? Or it is really the case that any of these exceptions could occur at any point in the execution of this code? Perhaps such refactoring is outside of the scope of this change, but I thought it was at least worth asking about. Line 862: result = self.imp_client.ping_impala_service() So this is a micro-optimization, but in general, I thinks it's cleaner to use tuple unpacking for something like this, i.e., rather than: vals = get_multiple_values() val_0 = val[0] val_1 = val[1] val_2 = val[2] you can also write: val_0, val_1, val_2 = get_multiple_values() -- To view, visit http://gerrit.cloudera.org:8080/3507 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I704eb64546e27c367830120241311fea6091266b Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-HasComments: Yes
