Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 )
Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13375/5/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/5/tests/query_test/test_kudu.py@1130 PS5, Line 1130: @SkipIfKudu.hms_integration_enabled I still think that a lot of the tests in this file and elsewhere that have been skipped don't need to be - eg. I'm not sure why this one wouldn't work as is with hms integration enabled. It also occurs to me - as written, this 'skip' won't ever actually be triggered in Impala's normal test runs, as we'll always run these tests against the default minicluster config, which doesn't enable HMS integration. Of course, even in that case people may use these tests to run in other situations, eg. there have been efforts to run them against real clusters, so it still makes sense to include the 'skip' for any tests that genuinely won't ever work with hms integration turned on. I'm wondering though if it would be worth the effort and testing time to parameterize these tests so that they all run both with and without hms integration enabled, eg. by having custom_cluster/TestKuduHMSIntegration inherent from some of these classes? A lot of the tests in this file really won't be impacted by the integration (eg. the scan tests) and it would be basically wasted test time to run them in both configurations, but a lot of them are affected (eg. this test is one I would definitely want to know works with HMS integration turned on) One question I have is what's Kudu plan for how this evolves in future version - i.e. is the non-HMS integrated functionality going to be aggressively deprecated, such that Impala will probably want to just make hms integration turned on the default config in our minicluster, or is the plan to maintain both code paths for awhile? Anyways, I think this patch has fairly good coverage as is, and we can probably just file a JIRA to clean some of this up later. -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 03 Jun 2019 18:09:57 +0000 Gerrit-HasComments: Yes
