Alexey Serbin 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 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/13400/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13400/2//COMMIT_MSG@11 PS2, Line 11: after the table is dropped in the : Kudu, relies on Kudu to drop the table in the HMS. Maybe, rephrase it a bit somehow? Right not it gives a hint on some order of events that contradicts with the actual one. 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@1508 PS2, Line 1508: KuduCatalogOpExecutor.dropTable(msTable, /*if exists*/ true); Does it wait for the table to be actually dropped from Kudu? 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: @pytest.mark.execute_serially While adding this attribute, could you justify on the reasoning in the doc comment for this method? I guess my question is what has changed that this method is now required to be run serially? http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@133 PS2, Line 133: @SkipIfKudu.no_hybrid_clock I'm not sure this test scenario requires hybrid clock to be present. Why would this test fail in the absence of hybrid clock? Could you add a comment clarifying on this into the method's doc comment? http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@134 PS2, Line 134: test_drop_non_empty_db Do you think it's worth adding a positive test scenario to cover the case when an empty database is dropped once all Kudu tables from the database have been dropped? http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@138 PS2, Line 138: db_name What if db_name is 'default' ? http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@150 PS2, Line 150: @SkipIfKudu.no_hybrid_clock Ditto for the reasoning to skip the test if no hybrid clock is around: why is it required? Please add a comment into the method's doc string. http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@153 PS2, Line 153: and that the managed tables are removed nit: move this into a separate sentence for easier comprehension and maybe rephrase into Make sure the corresponding managed tables are removed from Kudu. http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@155 PS2, Line 155: db_name What if db_name is 'default'? http://gerrit.cloudera.org:8080/#/c/13400/2/tests/custom_cluster/test_kudu.py@168 PS2, Line 168: assert not kudu_client.table_exists(kudu_table.name) Interesting, in case of Kudu+HMS systests which I ran on recent CDH6.x this fails since Kudu tables will be dropped upon notification from HMS. Has it been fixed already? If not, it will be necessary to do something like 'assert eventually' here. -- 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: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Tue, 28 May 2019 19:03:02 +0000 Gerrit-HasComments: Yes
