Michael Brown has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/4414/6/tests/common/kudu_test_suite.py File tests/common/kudu_test_suite.py: PS6, Line 47: super(KuduTestSuite, cls).setup_class() : if os.environ["KUDU_IS_SUPPORTED"] == "false": : pytest.skip("Kudu is not supported") Maybe move L47 to L50 (switch the ordering of these)? This isn't just a nit: it avoids running super.setup_class(), which is non-trivial, and then just deciding to skip after all. PS6, Line 51: @classmethod : def teardown_class(cls): : pass Not needed: 1. KuduTestSuite.setup_class() doesn't do anything that needs to be torn down here. 2. This overrides super.teardown_class() and doesn't call super.teardown_class(). super.teardown_class() (ImpalaTestSuite.teardown_class()) does do some work. So it seems safe to delete this method completely. http://gerrit.cloudera.org:8080/#/c/4414/6/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: PS6, Line 20: from kudu.client import INT32 ImportError: I think this needs to be kudu.schema PS6, Line 34: @classmethod : def add_test_dimensions(cls): : super(CustomClusterTestSuite, cls).add_test_dimensions() : cls.TestMatrix.add_constraint(lambda v: : v.get_value('table_format').file_format == 'parquet' and : v.get_value('table_format').compression_codec == 'none') I don't think this is needed: no tests here use a vector. PS6, Line 43: def test_kudu_master_addrs(self, cursor, kudu): kudu_client, not kudu. Note this isn't just a rename; it's to make use of the kudu_client fixture. There is no "kudu" fixture. PS6, Line 45: with self.temp_kudu_table(kudu, [INT32]) as kudu_table: kudu_client PS6, Line 50: props) SyntaxError: missing a closing paren here http://gerrit.cloudera.org:8080/#/c/4414/6/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: PS6, Line 27: from pprint import pprint Remove: unused import PS6, Line 30: from copy import copy : : from tests.beeswax.impala_beeswax import ImpalaBeeswaxException : from tests.common.impala_test_suite import ImpalaTestSuite : from tests.common.skip import SkipIf : from tests.common.test_dimensions import create_uncompressed_text_dimension : from tests.common.test_vector import TestDimension All imported but not used. PS6, Line 49: def test_kudu_scan_node(self, vector): This test is racy in exhaustive: two tests will be competing for the same database (kududb_test). You could play it safe and run it serially, but with a little bit of work, this test can be run in parallel if you 1. use the unique_database fixture L49 2. use_db=unique_database L52 3. Modify QueryTest/kudu-scan-node to access dimtbl using the fully qualified path (functional_kudu.dimtbl) 4. Remove all uses of kududb_test here and in kudu-scan-node.test https://gerrit.cloudera.org/#/c/4169/ shows some of the same principles as an example. -- To view, visit http://gerrit.cloudera.org:8080/4414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
