wangsheng has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14319 )

Change subject: IMPALA-8984: Fix race condition in creating Kudu table
......................................................................


Patch Set 4:

(8 comments)

Thanks for your reviewed, Quanlong. And I've already modify my patch by your 
suggestions.

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: Fix race condition in creating Kudu table
           :
> Could you sumarize the title to explain what this patch does? e.g. "Fix rac
Done


http://gerrit.cloudera.org:8080/#/c/14319/3//COMMIT_MSG@10
PS3, Line 10: create kudu managed table without Kudu-HMS Integration. This bug 
would
            : caused table deleted in Kudu storage, but reserved in HMS when 
multiple
            : identical queries submitted at the same time.
            :
> Could you add the explanation for how this patch fixs the problem? Though i
Done


http://gerrit.cloudera.org:8080/#/c/14319/3//COMMIT_MSG@15
PS3, Line 15: 'meta
> Is it possible to add a test to reproduce the problem?
This problem has been solved by this patch, and thus I cannot reproduce it. 
Even without this patch, it's hard to reproduce the bug in a test, because the 
multiple threads schedule is uncertain. I am debugging by setting a breakpoint 
at CatalogOpExecutor.java:2181 to reproduce the problem. First submit a query, 
when the breakpoint is reached, then submit the query again, resume the 
process, and the bug would happend.


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 CREA
The problem you mentioned above just like: kudu already exists a table, and we 
submitted a query 'CREATED IF NOT EXISTS' to create a same name table. This 
would return success, and we could opearte the table normally, Impala allows 
this situation happened. The only problem is that the original kudu table 
schema maybe different from the schema in query, in this case Impala would use 
kudu table schema as a unity, and thus user maybe get an unexpected table 
schema. In order to solved this problem, we need to modify the original 
processing logic, or just give a tip to notify user, maybe we could discuss in 
a new jira.


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
Done


http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2185
PS3, Line 2185: t
> nit: don't need space here
Done


http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2187
PS3, Line 2187:             } else {
> If the table has been created by other concurrent request, we should not re
Done


http://gerrit.cloudera.org:8080/#/c/14319/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2191
PS3, Line 2191:           }
> Same as above, if the table has been created by other concurrent request, d
Done



--
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: 4
Gerrit-Owner: wangsheng <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-Reviewer: wangsheng <[email protected]>
Gerrit-Comment-Date: Mon, 14 Oct 2019 08:12:26 +0000
Gerrit-HasComments: Yes

Reply via email to