Casey Ching 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 litt The lib64 problem got a little complicated so this stuff is consolidated now. 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? It's not needed anymore 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. Wow that's annoying. Thanks for checking. 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. Done 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 I think there are some things that depend on impala-python output not containing noise like this. These messages would be printed on every use of impala-py* and that seems excessive. Line 136: impala_toolchain_dir = os.environ.get("IMPALA_TOOLCHAIN") : if not impala_toolchain_dir: > Comparison with potential None, so: This will also detect an empty string which is fine. Line 184: os.makedirs(artifact_dir) > Can we clean this directory after the call of L191? Done 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. Done 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. The main difference is the db api/impyla client. I'd rather use that interface. Eventually all the tests should move to that since it supports both beeswax and hs2. I agree consolidating would be good. Line 245: def kudu(): > This should have a slightly more specific name, like kudu_client. Done Line 260: def conn(request): > This should also have a more specific name. What do you suggest? I think it's fine as-is. A generic "conn" would be an Impala connection, that doesn't seem like too far of a stretch. I'm open to changing though. 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? That would be very verbose. Being explicit might be helpful though. I ended up going with convenience. Line 292: def __unique_conn(db_name=None): > Very similar to unique_database. We should extend the unique_database funct Yes I was thinking they would be merged once testing has migrated to impyla. -- 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: Casey Ching <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
