Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17308 )
Change subject: IMPALA-10502: Handle CREATE/DROP events correctly ...................................................................... Patch Set 5: (7 comments) Still looking into details. Left some questions first. http://gerrit.cloudera.org:8080/#/c/17308/5/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/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1388 PS5, Line 1388: nit: redundant blank line http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1773 PS5, Line 1773: // this means that the database was recreated from some other application while : // this create database operation was in progress. We bail out by throwing an error : // in this case because it is possible the database which the user was trying to : // create was not the one which was eventually recreated in the metastore. : Preconditions.checkState(events.size() == 1, : "Database was recreated in metastore while " : + "createDatabase operation was in progress"); I'm thinking whether this is too strict. Maybe it's ok to not fail the ddl if the database finally exists. What's the downside if we use the latest database event? I think it would be useful for CREATE DATABASE IF NOT EXISTS commands. On the other side, for commands without the IF NOT EXISTS clause, users should parse the failure message to know that they should not retry in this case. And they have to wait for the latest CREATE event being applied (e.g. sleep a while), or trigger a INVALIDATE METADATA command (which is expensive) to see the db immediately and move forward. EDIT: Anyway, it's really a corner case to hit this. I'm ok with the current approach if it's simplest. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1802 PS5, Line 1802: if (!catalog_.isEventProcessingActive()) { : return null; : } nit: this can be one-line statement http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3107 PS5, Line 3107: it is possible that Kudu doesn't generate : // any metastore events. Do you mean Kudu could use a createTable API that doesn't generate HMS events? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3198 PS5, Line 3198: == Not related to this patch, but this looks buggy. Could you change it to equals() by the way? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3209 PS5, Line 3209: //TODO: Why don't we use the msTable above? Good point. I think this is what we should have changed in this patch: https://gerrit.cloudera.org/c/14515/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java or earlier in this patch: https://gerrit.cloudera.org/c/13399/21/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java Could you change it in this patch by the way? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3255 PS5, Line 3255: nit: redundant blank line -- To view, visit http://gerrit.cloudera.org:8080/17308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c5e96b48abac015240f20295b3ec3b1d71f24a Gerrit-Change-Number: 17308 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 22 Apr 2021 08:09:58 +0000 Gerrit-HasComments: Yes
