Quanlong Huang 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 16: (9 comments) Thanks for adjusting the test! I'm ok with the kudu ddl lock solution, since it won't be high concurrency in creating kudu tables in practise. Patch looks good to me now except some minor comments. http://gerrit.cloudera.org:8080/#/c/14319/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14319/14//COMMIT_MSG@9 PS14, Line 9: This patch fixed the race condition when use 'CREATE IF NOT EXISTS' to : 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. > I think we should explain the 'race condition' more clear. Also there's som Hope you could address on this. http://gerrit.cloudera.org:8080/#/c/14319/14//COMMIT_MSG@14 PS14, Line 14: Fix is to check whether table already exists in HMS after obtain the : 'metastoreDdlLock_' lock. If true, just return 'Table already exists' : tip, otherwise create a new table in HMS and then update catalog. > I think this is better: and this http://gerrit.cloudera.org:8080/#/c/14319/16/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14319/16/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@a98 PS16, Line 98: Why deleting this precondition check? http://gerrit.cloudera.org:8080/#/c/14319/16/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@89 PS16, Line 89: synchronized (kuduDdlLock_) { This synchronizes table creation for different tables. But looks like no other better and simpler choise. Please leave a comment above it mentioning that 'Acquire lock to protect table existence check and table creation'. http://gerrit.cloudera.org:8080/#/c/14319/16/tests/custom_cluster/test_concurrent_kudu_create.py File tests/custom_cluster/test_concurrent_kudu_create.py: PS16: Lake 5 more lines here about the license. http://gerrit.cloudera.org:8080/#/c/14319/16/tests/custom_cluster/test_concurrent_kudu_create.py@21 PS16, Line 21: tbl_name nit: const variable name should be in upper cases. http://gerrit.cloudera.org:8080/#/c/14319/16/tests/custom_cluster/test_concurrent_kudu_create.py@45 PS16, Line 45: 100 We can reduce this to a number that's enough to reproduce the bug without this patch. Maybe 20 or 50 is enough? So we can save some test time. http://gerrit.cloudera.org:8080/#/c/14319/16/tests/custom_cluster/test_concurrent_kudu_create.py@50 PS16, Line 50: time.sleep(0.5) I think we don't need these sleeps so the race conflicts will be more severe. What if we remove them? http://gerrit.cloudera.org:8080/#/c/14319/16/tests/custom_cluster/test_concurrent_kudu_create.py@61 PS16, Line 61: this bug We need to be more specifit, e.g. change it to IMPALA-8984 -- 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: 16 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: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: wangsheng <sky...@163.com> Gerrit-Comment-Date: Fri, 27 Dec 2019 12:21:29 +0000 Gerrit-HasComments: Yes
