impala-dev mailing list archives

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

(5 comments)

http://gerrit.cloudera.org:8080/#/c/2855/8/infra/python/bootstrap_virtualenv.py
File infra/python/bootstrap_virtualenv.py:

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
any)
> 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
changed


http://gerrit.cloudera.org:8080/#/c/2855/8/tests/conftest.py
File tests/conftest.py:

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e5e22b38d5bd09a36238e66a69aa42d1a941de7
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Casey Ching <casey@cloudera.com>
Gerrit-Reviewer: Casey Ching <casey@cloudera.com>
Gerrit-Reviewer: Ishaan Joshi <ishaan@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message