Michael Brown has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

Patch Set 6:


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 

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.

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:

PS6, Line 50:             props)
SyntaxError: missing a closing paren here

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 
            : 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 

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 <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to