Lars Volker has posted comments on this change. ( )

Change subject: IMPALA-6508: add KRPC test flag

Patch Set 5:


> Patch Set 5:
> 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, 
> unconditionally.
> 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 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 
cluster tests.

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.
File tests/common/
PS5, Line 137:     if pytest.config.option.test_krpc:
             :       cmd.append("--impalad_args=-use_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?
File tests/common/
PS5, Line 18: import os
> Unused import.
File tests/custom_cluster/
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
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Tue, 13 Feb 2018 17:25:59 +0000
Gerrit-HasComments: Yes

Reply via email to