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 <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Ishaan Joshi <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
