David Knupp 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) 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 explicitl > "impala is able to create tables with that compression and then > restarted without it enabled" Do you have any info for restarting impala without LZO enabled? > "It make make sense to call this _is_impalad_impalalzo_enabled() to > distinguish it from these potential other cases." Also, just wanted to clarify -- this method has been entirely removed as of patches 8 and 9. You're not lobbying for bringing it back, right? Line 257: impala_lzo_root = os.environ.get('IMPALA_LZO') > I think it would be a cleaner interface to add a py.test argument specifyin > "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" So, one thought in response to this suggestion (after some discussion with Michael B.) In general, we want our test runs to be as inclusive as possible be default. Making people specify an option to include LZO whenever they run impala-py.test or run-tests.py seems cumbersome, and seems like something that could be easily overlooked. I just wanted to bring that point up before making a change like this. > "I also suggest filing a feature request for Impala to have an interface > to return available compression schemes" This would be useful. Part of why this relatively simple change got so convoluted is that there is (apparently) no good way to way to test if Impala LZO is supported -- so it's been necessary to rely on imperfect checks of env variables, or .so files. 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 So just to reinforce what was decided earlier, after an off-line discussion with Dan on Friday, this method has been removed in favor of simply checking whether or not the IMPALA_LZO environment variable has been set. -- 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
