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 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/13386/8/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/13386/8/tests/common/environ.py@223 PS8, Line 223: def get_instance(cls): This is a non-blocking comment, but but there's a bit of python pedantry/hackery you can employ here to effect the Singleton pattern. Python has a __new__() method that gets called *before* __init__(). It rarely gets overridden, but for this case, you could do something like this to forgo needing a get_instance() class method: class ImpalaTestClusterProperties(object): _instance = None _build_flavor = None _library_link_type = None _web_ui_url = None def __new__(cls): if cls._instance is None: cls._web_ui_url = IMPALA_REMOTE_URL or DEFAULT_LOCAL_WEB_UI_URL if IMPALA_REMOTE_URL: # If IMPALA_REMOTE_URL is set, prefer detecting from the web UI. cls._build_flavor, cls._library_link_type =\ ImpalaTestClusterFlagsDetector.detect_using_web_ui(web_ui_url) else: cls._build_flavor, cls._library_link_type =\ ImpalaTestClusterFlagsDetector.detect_using_build_root_or_web_ui(IMPALA_HOME) cls._instance = object.__new__(cls) return cls._instance def __init__(self): self._runtime_flags = None # Lazily populated to avoid unnecessary web UI calls. @property def build_flavor(self): """ Return the correct ImpalaBuildFlavors for the Impala under test. """ return self._build_flavor @property def library_link_type(self): """ Return the library link type (either static or dynamic) for the Impala under test. """ return self._library_link_type # etc... >>> instance1 = ImpalaTestClusterProperties() >>> instance2 = ImpalaTestClusterProperties() >>> >>> print id(instance1) 4378407440 >>> print id(instance2) 4378407440 >>> instance1 == instance2 True http://gerrit.cloudera.org:8080/#/c/13386/8/tests/common/environ.py@321 PS8, Line 321: def _get_flags_from_web_ui(self): Also non-blocking, but a style nit here: seems like this would be a good place to use the @property decorator, which generally gets used where attributes need to be lazily eval'ed? @property def runtime_flags(self): if self._runtime_flags is None: # do stuff return self._runtime_flags Then later... def is_catalog_v2_cluster(self): """Whether we use CATALOG_V2 options, including local catalog and HMS notifications. For now, assume that --use_local_catalog=true implies that the others are enabled.""" try: key = "use_local_catalog" # --use_local_catalog is hidden so does not appear in JSON if disabled. return key in self.runtime_flags and self.runtime_flags[key]["current"] == "true" except Exception: if self.is_remote_cluster(): # IMPALA-8553: be more tolerant of failures on remote cluster builds. LOG.exception("Failed to get flags from web UI, assuming catalog V1") return False raise http://gerrit.cloudera.org:8080/#/c/13386/8/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/13386/8/tests/hs2/test_hs2.py@42 PS8, Line 42: handless handles? -- 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: 8 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 19 Jun 2019 18:38:22 +0000 Gerrit-HasComments: Yes
