impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (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 00:36:57 GMT
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:

Funnily enough, building Impala LZO did fail the first time I tried to verify this (because
I'd forgotten to rebase first):

http://github.mtv.cloudera.com/gist/dknupp/7b30d18f6a71a6174021

...but that caused the whole build to fail. For what it's worth.

I think my understanding of Dan's comment is slightly different. He mentioned "the scenario
where we rename the lzo so for some reason and lose test coverage because of that." Which
implies to me that building LZO is successful, but that the assumption being used to check
for LZO (namely, that ${IMPALA_LZO}/build/libimpalalzo.so exists) no longer holds true, so
we wind up skipping a bunch of tests that should have been run. Did I completely get that
wrong?

I'm not sure what moving the logic for that to a separate script would buy us. (But perhaps
I'm missing your point Tim?)

Maybe I just need to be less specific in _is_impalalzo_present(). Maybe if the directory ${IMPALA_LZO}
exists, that's good enough to run LZO tests. If there's been a silent build failure for some
reason, tests will still fail.

Only if ${IMPALA_LZO} isn't a directory at all, then it's safe to skip the tests, with the
assumption at that point being that the tests are being run by someone (an EXTERNAL someone)
who didn't have Impala LZO because they only have access to the ASF repo.

-- 
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: 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: No

Mime
View raw message