kis...@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@444 PS3, Line 444: tableInfo.toString()); Can you wrap the lines from 435 to 444 within an if isDebugEnabled ? http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@457 PS3, Line 457: // except last Why you are you releasing the write locks here ? http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@186 PS3, Line 186: // long newCatalogVersion = catalog_.incrementAndGetCatalogVersion(); Please remove the line, if you are not planning to use newCatalogVersion? http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@192 PS3, Line 192: catalogOpExecutor_.syncToLatestEventId(Db db, apiName); Can you add one sample test case, where CatalogOpExecutor and MetastoreServiceHanlder interact with the same table at the same time ? I just want to see how it would look like. http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@219 PS3, Line 219: catalog_.getLock().writeLock().unlock(); Do we still need Catalog level lock, since we now have global table level lock ? http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@247 PS3, Line 247: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(), > line too long (92 > 90) Please take care of these. http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@396 PS3, Line 396: catalogOpExecutor_.UnlockWriteLockIfErronouslyLocked(); UnlockWriteLockIfErronouslyLocked what does this mean ? http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@411 PS3, Line 411: CatalogException e = Can you not throw TException ? How CatalogException is handled up in the chain ? http://gerrit.cloudera.org:8080/#/c/17703/3/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/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1401 PS3, Line 1401: protected void throwException(Exception cause, String apiName) May be rethrowException ? http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2871 PS3, Line 2871: private long getCurrentEventId(MetaStoreClient msClient) throws TException { Where are you using this ? -- To view, visit http://gerrit.cloudera.org:8080/17703 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361 Gerrit-Change-Number: 17703 Gerrit-PatchSet: 3 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: Wed, 21 Jul 2021 16:54:57 +0000 Gerrit-HasComments: Yes
