impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1671: Print time and link to coordinator web UI once query is submitted in shell
Date Mon, 27 Jun 2016 23:47:36 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message