Ishaan Joshi has posted comments on this change. Change subject: Add Kudu test helpers ......................................................................
Patch Set 8: (5 comments) Looked at bootstrap, will take a look at fixtures next. 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 already very long. So maybe just install_kudu_client() 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 invoke python in this manner. Line 157: reqs_file = open(REQS_PATH) Maybe use the 'with' context manager to avoid the explicit close() Line 196: except: Do we want naked excepts? We're prone to it catching KeyBoardInterrupt 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 -- 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
