impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) Add Kudu test helpers
Date Mon, 25 Apr 2016 23:25:22 GMT
Michael Brown has posted comments on this change.

Change subject: Add Kudu test helpers

Patch Set 1:

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
File bin/impala-py.test:

Line 7: # Update the library path to find libkudu.
If impala-python is already doing this, why do you need this again?
File bin/impala-python:

No such file or directory on my machine.

After I ran I found here:

File infra/python/

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

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.
File tests/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e5e22b38d5bd09a36238e66a69aa42d1a941de7
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Casey Ching <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-HasComments: Yes

View raw message