impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Date Mon, 17 Oct 2016 17:29:29 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message