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 <dkn...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Harrison Sheinblatt <h...@hotmail.com> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
