Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13933 )
Change subject: IMPALA-8627: Enable catalog-v2 in tests ...................................................................... Patch Set 11: (4 comments) I think this generally makes sense. I had a couple of questions about the test and a suggestion to avoid some code duplication. http://gerrit.cloudera.org:8080/#/c/13933/11/tests/hs2/hs2_test_suite.py File tests/hs2/hs2_test_suite.py: http://gerrit.cloudera.org:8080/#/c/13933/11/tests/hs2/hs2_test_suite.py@74 PS11, Line 74: def add_session(self, cluster_properties): I think we can avoid all of the duplication with the above add session function if we define a helper function that receives all the required arguments and the function to invoke in the middle, i.e. def add_session_helper(self, protocol_version, conf_overlay, close_session, fn): ... fn() ... Then we could invoke it with something like add_session_helper(self, protocol_version, conf_overlay, close_session, lambda: fn(self, cluster_properties)) I tried it out here - seems to work: https://github.com/timarmstrong/impala/tree/refactor-session http://gerrit.cloudera.org:8080/#/c/13933/11/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/13933/11/tests/hs2/test_hs2.py@440 PS11, Line 440: @needs_session_cluster_properties() If we're defining a new decorator anyway, could we pass in unique_database too and remove execute_serially? That was a workaround for the decorator issue, but it looks like you might have figured it out. http://gerrit.cloudera.org:8080/#/c/13933/11/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/13933/11/tests/query_test/test_observability.py@318 PS11, Line 318: r'CatalogFetch.Config.Misses|CatalogFetch.Config.Hits', Is it guaranteed that we always get one or the other? http://gerrit.cloudera.org:8080/#/c/13933/11/tests/query_test/test_observability.py@340 PS11, Line 340: self.__verify_profile_event_sequence(load_event_regexes, runtime_profile) Do we need to test that the above regexes are in sequential lines, or just that they're present? Originally this was meant to assert that the events were in the specific order. I guess the advantage of this is that we'll know to update the test when new metrics are added. So seems OK. -- To view, visit http://gerrit.cloudera.org:8080/13933 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iddbde666de2b780c0e40df716a9dfe54524e092d Gerrit-Change-Number: 13933 Gerrit-PatchSet: 11 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Tue, 06 Aug 2019 06:30:39 +0000 Gerrit-HasComments: Yes
