Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14319 )
Change subject: IMPALA-8984: Submit several same kudu table created query concurrently may result in table deleted in Kudu storage ...................................................................... Patch Set 3: (8 comments) Nice find! I can understand the bug, but have some questions for the patch. http://gerrit.cloudera.org:8080/#/c/14319/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14319/3//COMMIT_MSG@7 PS3, Line 7: Submit several same kudu table created query : concurrently may result in table deleted in Kudu storage Could you sumarize the title to explain what this patch does? e.g. "Fix race condition in creating Kudu table". The title of the commit message should not be the JIRA title while just explains the problem. http://gerrit.cloudera.org:8080/#/c/14319/3//COMMIT_MSG@10 PS3, Line 10: If several users submitted a same kudu table created query : concurrently, the result could be: All these queries : finished, and table created in HMS, but not exists : in kudu storage. Could you add the explanation for how this patch fixs the problem? Though it's simple but it's not good to just copy the JIRA description here... BTW, could you specify which kind of tables are created? e.g. managed table, external table? with or without Kudu HMS integration? I guess the queries should also contains the IF NOT EXISTS clause. http://gerrit.cloudera.org:8080/#/c/14319/3//COMMIT_MSG@15 PS3, Line 15: Tests Is it possible to add a test to reproduce the problem? http://gerrit.cloudera.org:8080/#/c/14319/3/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/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2170 PS3, Line 2170: KuduCatalogOpExecutor.createManagedTable(newTable, params); I think we have race here too. If two different users sending the same CREATE TABLE IF NOT EXISTS statement to create a managed kudu table, it's possible that userA creates it in Kudu and userB creates it in HMS. So the ownership is inconsistent. http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2182 PS3, Line 2182: nit: too more spaces http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2185 PS3, Line 2185: nit: don't need space here http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2187 PS3, Line 2187: } If the table has been created by other concurrent request, we should not return true. Otherwise, users will consider their statements succeed and they should be the owner (and have owner privileges). But actually only the first user is the owner. http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2191 PS3, Line 2191: Table newTbl = catalog_.addTable(newTable.getDbName(), newTable.getTableName()); Same as above, if the table has been created by other concurrent request, don't need to create a new catalog object here. -- To view, visit http://gerrit.cloudera.org:8080/14319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a4047bcdaa6b346765b96e8c36bb747a2b0091d Gerrit-Change-Number: 14319 Gerrit-PatchSet: 3 Gerrit-Owner: wangsheng <sky...@163.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 11 Oct 2019 07:38:19 +0000 Gerrit-HasComments: Yes
