David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/13386 )
Change subject: IMPALA-8553,IMPALA-8552: fix checks for remote cluster ...................................................................... Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13386/3/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/13386/3/tests/common/environ.py@332 PS3, Line 332: else: I wonder if there's some way to salvage this a bit. I think ultimately, the idea of consolidating on using IMPALA_TEST_CLUSTER_PROPERTIES.is_remote_cluster() is a step in the right direction. What if we did something thing like this: build_type = LOCAL_BUILD web_ui_url = DEFAULT_LOCAL_WEB_UI_URL build_flavor, link_type =\ ImpalaTestClusterFlagsDetector.detect_using_build_root_or_web_ui(IMPALA_HOME) if IMPALA_REMOTE_URL: build_type = REMOTE_BUILD web_ui_url = IMPALA_REMOTE_URL build_flavor, link_type =\ ImpalaTestClusterFlagsDetector.detect_using_web_ui(IMPALA_REMOTE_URL) elif os.getenv("REMOTE_LOAD") or pytest.config.option.testing_remote_cluster: build_type = REMOTE_BUILD IMPALA_TEST_CLUSTER_PROPERTIES =\ ImpalaTestClusterProperties(build_flavor, link_type, build_type) Then you can leave shell/util.py untouched. FWIW, some observations re: code/architecture smells. For one, we weirdly have data load happening as a side effect of our bash build scripts, rather than occurring as a setup step in our test framework. This requires us to set alternate env vars like REMOTE_LOAD. If we instead loaded data using, say, a session scoped test fixture -- a common task for test fixtures -- then it really would be easier to have environ.py act as a common point of reference. Also, passing parameters to python by use of environment variable is equally frustrating. (There's a general over-reliance/abuse of global variables throughout.) -- To view, visit http://gerrit.cloudera.org:8080/13386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifa6b2a1391f53121d3d7c00c5cf0a57590899ce4 Gerrit-Change-Number: 13386 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 29 May 2019 20:42:57 +0000 Gerrit-HasComments: Yes
