Sourabh Goyal 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 4: (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 ? Sure ! 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 ? So that the caller of this method has to call unlock() only once. 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: } > Please remove the line, if you are not planning to use newCatalogVersion? Ack http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@192 PS3, Line 192: LOG.debug("Successfully executed HMS API: " + apiName); > Do we need to sync table/database to latest event in this class? If we don' @Kishen: Sure, will add a UT. For now it would be a no-op since we haven't implemented syncToLatestEventId() yet. I was planning to add tests in implementation patch of syncToLatestEventId(). @Yu-Wen: Why should we defer the update? If we do so, then it is possible that we may have to sync up lots of HMS events in the next read leading to delays in the read. However if we sync up with every HMS operation then we may have to apply only small number of updates. http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@219 PS3, Line 219: // get released in finally block > Do we still need Catalog level lock, since we now have global table level l Rename requires removing an old table and adding back a new table in cache, that's why global write lock is required to make sure that cache remains unchanged during this operation. http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@247 PS3, Line 247: String apiName = HmsApiNameEnum.ADD_PARTITION.apiName(); > Please take care of these. Ack http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@396 PS3, Line 396: } finally { > UnlockWriteLockIfErronouslyLocked what does this mean ? It forcefully unlocks the version.writeLock() if the lock was held by current thread by this time. It is just for precaution to make sure that version lock is always released in the end since it is a global lock. http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@411 PS3, Line 411: // should it be an internal exception? > Can you not throw TException ? at line no. 414 - throwException() throws MetaException - a type of TException. 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 ? Sure, will rename it. 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 ? For now, I am not using it anywhere in the patch and the implementation already existed. I have just moved it up. -- 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: 4 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 10:45:55 +0000 Gerrit-HasComments: Yes
