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 14: > > > > > (1 comment) > > > > > > > > Lock both kudu table creation and hms table creation by > > > 'DdlLock_' > > > > can certainly solve this problem, but maybe to heavy. > > > > It doesn't matter that KuduCatalogOpExecutor.createManagedTable > > > is > > > > not thread-safey, because the root reason for this bug is: > > create > > > > table in hms failed(first query already created) caused an > > > > exception which lead to table delete in kudu storage(line > > 2254). > > > > Even two threads create table in kudu stoage at the same > time, > > > kudu > > > > provide guarantee to this situation, and it's doesn't matter > > > which > > > > query created table in kudu finally. So I just add the code > to > > > > check whether table already exist in hms. > > > > > > I think what I meant was if there are 2 threads which try to > > create > > > a Kudu table with the same name, one of them might succeed > while > > > other will fail at > > > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java#L101 > > > > > > This error will be thrown as ImpalaRuntimeException which is > will > > > fail the query. If the query specifies create if not exists, > > > ideally it should be idempotent. It may not be the exact > problem > > > that this patch is fixing but it is close enough to the race > > > condition described and it would be good to handle this case as > > > well. > > > > Thanks for your kindly review and reply, Vihang. I understand, > you > > mean two thread create same table, and 'kudu.createTable' is not > > thread-safety, this may caused ImpalaRuntimeException without 'if > > not exists'. I've already read the related code, and here is my > > opion: > > Firstly, not only create, but drop/rename/alter all exist the > > problem you mentioned above, we need to add lock in all these > > situation. And for these situations, more test cases need to be > > written to verify the function, is it better to deal with these > > issues in another jira? > > Secondly, this bug is not very similar to those issues > > describe(kudu create/drop/rename/alter), maybe 'race condition' > is > > not very exactly for this jira. > > How do you think? > > Besides, I'm not sure that my fe test is suitable. Quanlong Huang > > suggest me to write an e2e test by python. But I cannot reporduce > > this bug with an e2e test case, due to thread schedule path are > > random, only specific path can lead to this bug, so I've no idea > > about this. > > I am okay with fixing the other race condition with respect to kudu > create table statement erroring out if that works better for you. Thanks for understanding, would you please give me a cr+1? > > > > > (1 comment) > > > > > > > > Lock both kudu table creation and hms table creation by > > > 'DdlLock_' > > > > can certainly solve this problem, but maybe to heavy. > > > > It doesn't matter that KuduCatalogOpExecutor.createManagedTable > > > is > > > > not thread-safey, because the root reason for this bug is: > > create > > > > table in hms failed(first query already created) caused an > > > > exception which lead to table delete in kudu storage(line > > 2254). > > > > Even two threads create table in kudu stoage at the same > time, > > > kudu > > > > provide guarantee to this situation, and it's doesn't matter > > > which > > > > query created table in kudu finally. So I just add the code > to > > > > check whether table already exist in hms. > > > > > > I think what I meant was if there are 2 threads which try to > > create > > > a Kudu table with the same name, one of them might succeed > while > > > other will fail at > > > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java#L101 > > > > > > This error will be thrown as ImpalaRuntimeException which is > will > > > fail the query. If the query specifies create if not exists, > > > ideally it should be idempotent. It may not be the exact > problem > > > that this patch is fixing but it is close enough to the race > > > condition described and it would be good to handle this case as > > > well. > > > > Thanks for your kindly review and reply, Vihang. I understand, > you > > mean two thread create same table, and 'kudu.createTable' is not > > thread-safety, this may caused ImpalaRuntimeException without 'if > > not exists'. I've already read the related code, and here is my > > opion: > > Firstly, not only create, but drop/rename/alter all exist the > > problem you mentioned above, we need to add lock in all these > > situation. And for these situations, more test cases need to be > > written to verify the function, is it better to deal with these > > issues in another jira? > > Secondly, this bug is not very similar to those issues > > describe(kudu create/drop/rename/alter), maybe 'race condition' > is > > not very exactly for this jira. > > How do you think? > > Besides, I'm not sure that my fe test is suitable. Quanlong Huang > > suggest me to write an e2e test by python. But I cannot reporduce > > this bug with an e2e test case, due to thread schedule path are > > random, only specific path can lead to this bug, so I've no idea > > about this. > > I am okay with fixing the other race condition with respect to kudu > create table statement erroring out if that works better for you. Thanks for your understanding, would please give me a cr+1? -- 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: 14 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Tue, 24 Dec 2019 08:03:52 +0000 Gerrit-HasComments: No
