Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9291 )
Change subject: IMPALA-6508: add KRPC test flag ...................................................................... Patch Set 5: (3 comments) > 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 > KRPC > > 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 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 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. http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/custom_cluster_test_suite.py@137 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? http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/test_skip.py File tests/common/test_skip.py: http://gerrit.cloudera.org:8080/#/c/9291/5/tests/common/test_skip.py@18 PS5, Line 18: import os > Unused import. Done http://gerrit.cloudera.org:8080/#/c/9291/5/tests/custom_cluster/test_krpc_mem_usage.py File tests/custom_cluster/test_krpc_mem_usage.py: http://gerrit.cloudera.org:8080/#/c/9291/5/tests/custom_cluster/test_krpc_mem_usage.py@77 PS5, Line 77: @CustomClusterTestSuite.with_args("--stress_datastream_recvr_delay_ms=1000") > 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-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab Gerrit-Change-Number: 9291 Gerrit-PatchSet: 5 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 Gerrit-HasComments: Yes
