Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13400 )
Change subject: IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1439 PS2, Line 1439: // The Kudu tables in the HMS should have been dropped at this point : // with the Hive Metastore integrat > Shouldn't this be true regardless of HMS integration? Isn't that what dropT Sorry for the confusion, I mean the Kudu table in the HMS should be deleted at this pointed. http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1508 PS2, Line 1508: KuduCatalogOpExecutor.dropTable(msTable, /*if exists*/ true); > Does it wait for the table to be actually dropped from Kudu? Are you referring to wait for the table to be actually dropped in the HMS when HMS integration is enabled? I think this is not necessary. The table should be dropped in the HMS if dropping the table in Kudu returns successfully. http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1512 PS2, Line 1512: private boolean isKuduHmsIntegrationEnab > The CatalogOpExecutor class isn't Kudu-specific; this should probably be na Done http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1613 PS2, Line 1613: // When Kudu's HMS integration is enabled, Kudu will drop the managed table > Please add a comment explaining the rationale of this condition, eg Done http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1616 PS2, Line 1616: if (!isManagedKuduTable || !isKuduHmsIntegrationEnabled(msTbl)) { > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@124 PS2, Line 124: > While adding this attribute, could you justify on the reasoning in the doc My understanding is that all tests in under custom_cluster dir should run serially because all of them requires restart certain components with customized configurations. http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@133 PS2, Line 133: def teardown_class(cls): > I'm not sure this test scenario requires hybrid clock to be present. Why w Yeah, I think this is actually not needed. http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@138 PS2, Line 138: > What if db_name is 'default' ? I think the unique_cursor ensured the db name is unique. http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@150 PS2, Line 150: """ > Ditto for the reasoning to skip the test if no hybrid clock is around: why Done http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@153 PS2, Line 153: elf.get_kudu_table_base_name(kudu_table. > nit: move this into a separate sentence for easier comprehension and maybe Done http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@155 PS2, Line 155: table_n > What if db_name is 'default'? Same as above. http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@168 PS2, Line 168: assert ["", "storage_handler", "org.apache.kudu.hive > Interesting, in case of Kudu+HMS systests which I ran on recent CDH6.x this I don't quite understand why you encountered scenario that after dropping the table in Impala, the kudu table is dropped after some time. Because drop table in Impala will call Kudu drop table API explicitly which should be a sync call. Would you mind trying again with the latest bits and with my patches included? Thanks! -- To view, visit http://gerrit.cloudera.org:8080/13400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Gerrit-Change-Number: 13400 Gerrit-PatchSet: 3 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 07:11:05 +0000 Gerrit-HasComments: Yes
