Lars Volker has posted comments on this change. (
Change subject: IMPALA-6508: add KRPC test flag
Patch Set 5:
> Patch Set 5:
> (3 comments)
> I was trying to think about how this could be done without yet another
> "global" environment variable, and an unenforceable flag between pytest and
> the cluster, and more skip logic.
> Do you think it's possible to achieve the same effect by:
> 1. Creating CI that sets TEST_START_CLUSTER_ARGS appropriately for enabling
> 2. Ensuring $TEST_START_CLUSTER_ARGS is properly reconciled any time the
> cluster starts, even in custom cluster tests.
> 3. Running the 3 tests that are KRPC specific with --use-krpc always set,
> I have a feeling this would reduce some of the churn that's introduced in
> this patch and then would be gutted again later. What do you think?
I think we plan on keeping the option to turn KRPC off, even after we turn it
on by default. This means we will have automated test runs with both krpc
enabled and disabled for the foreseeable future, until we completely remove the
old Thrift RPC code. It would be very convenient to add tests into the
appropriate test files, e.g. add a test for KRPC metrics showing up on the
/rpcz page (IMPALA-6269) to test_web_pages.py. In these files we would need a
way to decorate tests that require KRPC and should be skipped for runs with
KRPC disabled. Similarly I anticipate that we will have tests that require
Thrift RPC at some time. Our e2e tests should also be faster than the custom
Not having a SkipIf means we would have to look at the environment to skip
those tests from within the test methods, but wrapping that in a function then
seems to be equivalent of having a SkipIf.
Hopefully we will also have a lot more than 3 tests that require KRPC to be
turned on. :)
We could try to interfere the SkipIf from the command line argument to reduce
the amount of config variables. Alternatively we could unhide -use_krpc and get
the value from the /varz page.
PS5, Line 137: if pytest.config.option.test_krpc:
> It seems like a bug that custom cluster tests don't prepend $TEST_START_CLU
I'm not sure I understand what you mean. Should they use
$TEST_START_CLUSTER_ARGS instead of appending to the command line themselves?
PS5, Line 18: import os
> Unused import.
PS5, Line 77:
> An alternative to skipping would be to just force --use-krpc always to on i
I'll reply on the change itself.
To view, visit http://gerrit.cloudera.org:8080/9291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 17:25:59 +0000