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