You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Ishaan Joshi (Code Review)" <ge...@cloudera.org> on 2016/05/04 00:38:05 UTC
[Impala-CR](cdh5-trunk) Add Kudu test helpers
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 <ca...@cloudera.com>
Gerrit-Reviewer: Casey Ching <ca...@cloudera.com>
Gerrit-Reviewer: Ishaan Joshi <is...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes