impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
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:

(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 <casey@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message