impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Casey Ching (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) Add Kudu test helpers
Date Thu, 12 May 2016 22:43:14 GMT
Casey Ching has posted comments on this change.

Change subject: Add Kudu test helpers

Patch Set 8:

File infra/python/

Line 133:   if os.environ["KUDU_IS_SUPPORTED"] != "true":
> Might be better to move this validation step out of this method, it's alrea
I kept it like this so all the skip conditions are together. They could all be factored out
but this way its clear that no other functions use this logic.

Line 146:   # The "pip" command could be used to provide the version of Kudu installed (if
> Can you add a further comment to this, i.e, explaining why you have to invo
I changed the comment a little hopefully its clearer. If thats not what you had in mind let
me know what you were expecting.

Line 157:   reqs_file = open(REQS_PATH)
> Maybe use the 'with' context manager to avoid the explicit close()
This needs to work with python 2.4 and "with" isnt there.

Line 196:     except:
> Do we want naked excepts? We're prone to it catching KeyBoardInterrupt
File tests/

Line 212:   'unique_curor' fixtures below.
> Nit: s/unique_curor/unique_cursor

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e5e22b38d5bd09a36238e66a69aa42d1a941de7
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Casey Ching <>
Gerrit-Reviewer: Casey Ching <>
Gerrit-Reviewer: Ishaan Joshi <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-HasComments: Yes

View raw message