Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17713 )
Change subject: IMPALA-10817: Share metastoreHmsDDL lock b/w CatalogOpExecutor and Catalog metastore server ...................................................................... Patch Set 1: (3 comments) I took a first pass and the approach makes sense to me. I see you have a separate change to handle the table lock which is good. http://gerrit.cloudera.org:8080/#/c/17713/1/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17713/1/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@359 PS1, Line 359: // Lock used to ensure that CREATE[DROP] TABLE[DATABASE] operations performed in : // catalog_ and the corresponding RPC to apply the change in HMS are atomic. : // This lock is shared b/w HMS operations performed in CatalogOpExecutor : // and Metastore server nit, can you move this to line 319 where the field is declared. http://gerrit.cloudera.org:8080/#/c/17713/1/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@431 PS1, Line 431: metastoreHmsDdlLock_ In my opinion it is more readable to do instead of creating a variable which points to the same lock eventually. If this is too verbose for your preference you can also create a method like takeDdlLock() and releaseDdlLock(); catalogOpExecutor_.getDdlLock().lock(); catalogOpExecutor_.getDdlLock().unlock(); http://gerrit.cloudera.org:8080/#/c/17713/1/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/17713/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5643 PS1, Line 5643: } finally { : getMetastoreDdlLock().unlock(); : } you can use the same try block as in line 5632. -- To view, visit http://gerrit.cloudera.org:8080/17713 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60d4f3a49eb843fa8640cd21d623fd8dda770001 Gerrit-Change-Number: 17713 Gerrit-PatchSet: 1 Gerrit-Owner: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <kis...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Comment-Date: Thu, 22 Jul 2021 18:58:00 +0000 Gerrit-HasComments: Yes
