impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Harrison Sheinblatt (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3898: Add a pytest skipif decorator based on presence of Impala LZO.
Date Fri, 12 Aug 2016 21:29:01 GMT
Harrison Sheinblatt has posted comments on this change.

Change subject: IMPALA-3898: Add a pytest skipif decorator based on presence of Impala LZO.
......................................................................


Patch Set 7:

(3 comments)

Suggestions added

http://gerrit.cloudera.org:8080/#/c/3782/7/tests/common/environ.py
File tests/common/environ.py:

Line 252: def _is_impalalzo_present():
I don't think we have negative or negative Hive integration tests explicitly yet, but there
are potentially negative test cases where Hive is enabled to use impala-lzo and tables can
exist with that compression but impala is not, or impala is able to create tables with that
compression and then restarted without it enabled.  Then we'd test that the errors returned
by Impala when accessing such a table are meaningful.

It make make sense to call this _is_impalad_impalalzo_enabled() to distinguish it from these
potential other cases.  Suggest also filing feature requests to add the above negative tests
if they don't exist.

These tests are lower priority since it seems unlikely one would use impala-lzo in Hive and
not Impala.  The reverse seems more likely but still not very likely.


Line 257:   impala_lzo_root = os.environ.get('IMPALA_LZO')
I think it would be a cleaner interface to add a py.test argument specifying compression schemes
to test that defaulted to not include impala-lzo.  If we do need to depend explicitly on environment
variables, not just py.test args, then it would be good to document those dependencies in
the py.test cmd line help.  I also suggest filing a feature request for Impala to have an
interface to return available compression schemes, e.g. a SQL call that returned allowed codecs,
and referencing it here.


Line 260:   return os.path.isfile(os.path.join(impala_lzo_root, 'build', 'libimpalalzo.so'))
This is going to fail for remote clusters, so suggest adding a comment (or ref to feature
request) that's searchable later so we can find known places to update.


-- 
To view, visit http://gerrit.cloudera.org:8080/3782
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If61a7799205cd00d440196303a42db32c522f5b1
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs7@hotmail.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message