Michael Brown has posted comments on this change. Change subject: Add Kudu test helpers ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/2855/1/bin/impala-ipython File bin/impala-ipython: Line 6: It would be great if we could avoid some of the small DRYness in these little infra environment-proxying scripts. http://gerrit.cloudera.org:8080/#/c/2855/1/bin/impala-py.test File bin/impala-py.test: Line 7: # Update the library path to find libkudu. : LD_LIBRARY_PATH+=":$IMPALA_TOOLCHAIN/kudu-$IMPALA_KUDU_VERSION/debug/lib64" If impala-python is already doing this, why do you need this again? http://gerrit.cloudera.org:8080/#/c/2855/1/bin/impala-python File bin/impala-python: Line 5: export LD_LIBRARY_PATH+=":$IMPALA_TOOLCHAIN/kudu-$IMPALA_KUDU_VERSION/debug/lib64" No such file or directory on my machine. After I ran bootstrap_toolchain.py I found libkudu_client.so.0.1.0 here: $IMPALA_TOOLCHAIN/kudu-$IMPALA_KUDU_VERSION/debug/lib http://gerrit.cloudera.org:8080/#/c/2855/1/infra/python/bootstrap_virtualenv.py File infra/python/bootstrap_virtualenv.py: Line 69: 'args' and 'kwargs' use the same format as subprocess.Popen(). Also the method now returns the contents of stdout. Line 134: LOG.debug("Skipping Kudu: Kudu is not supported") It would be helpful if these were INFO-level, at least for some weeks while this patch bakes in. Line 136: impala_toolchain_dir = os.environ.get("IMPALA_TOOLCHAIN") : if not impala_toolchain_dir: Comparison with potential None, so: if impala_toolchain_dir is None: Line 184: os.makedirs(artifact_dir) Can we clean this directory after the call of L191? Line 189: env["LIBRARY_PATH"] = os.path.join(os.environ["IMPALA_TOOLCHAIN"], : "kudu-%s" % os.environ["IMPALA_KUDU_VERSION"], "debug", "lib64") Again, this was just lib on my machine, not lib64. http://gerrit.cloudera.org:8080/#/c/2855/1/tests/conftest.py File tests/conftest.py: Line 242: Some of this stuff competes with the unique_database fixture functionality. if unique_database doesn't suit your needs then we should extend it or prefer this one to that and remove unique_database. Line 245: def kudu(): This should have a slightly more specific name, like kudu_client. Line 260: def conn(request): This should also have a more specific name. Line 272: db_name = __call_cls_method_if_exists(request.cls, "get_db_name") : use_unique_conn = __call_cls_method_if_exists(request.cls, "auto_create_db") Why not pass parameters to the fixture instead of doing it this way? Line 292: def __unique_conn(db_name=None): Very similar to unique_database. We should extend the unique_database functionality if this doesn't work, or we should remove unique_database and update all the tests that use it to use this. I don't think we should have both. -- 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: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
