kis...@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17859 )
Change subject: IMPALA-10926: Sync db/table in catalog cache to latest HMS event id when performing DDL operations via catalog HMS endpoints ...................................................................... Patch Set 14: (20 comments) http://gerrit.cloudera.org:8080/#/c/17859/14/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/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@373 PS14, Line 373: Extra line http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@436 PS14, Line 436: * Acquire write lock on multiple tables If the lock couldn't be acquired on any multiple tables. If the http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@440 PS14, Line 440: * @return true if lock was acquired on all tables successfully. False true if lock was acquired on all tables successfully; false otherwise. http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@468 PS14, Line 468: // except last Why except last, can you add that in the comment itself ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@470 PS14, Line 470: versionLock_.writeLock().unlock(); Is it possible to get some exception here where we end up with some tables that are still locked ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2261 PS14, Line 2261: extra line http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2661 PS14, Line 2661: // TODO: If reloadTable succeeds, should we sync table till current HMS Yes, we should sync it to the latest. Please address all TODO before submit it again for review. http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3658 PS14, Line 3658: public MetastoreEventFactory getEventFactoryForSyncToLatestEvent() { white line missing between methods. http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/Table.java@199 PS14, Line 199: // TODO: Get rid of get and createEventId Will this TODO be done as part of this patch ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/Table.java@201 PS14, Line 201: // last synced id in full table reload last or latest ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/Table.java@212 PS14, Line 212: public void setLastSyncedEventId(long eventId) { white line missing between methods. http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@57 PS14, Line 57: private Metrics metrics_ = new Metrics(); Where are you publishing or logging this ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java File fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java: http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java@165 PS14, Line 165: tblLoader_ = new TableLoader(catalog); Why is this change required ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@840 PS14, Line 840: protected boolean shouldSkipWhenSyncingToLatestEventId() throws CatalogException { Can it not be a separate utility method, which takes eventId, dbName and table name as arguments ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@329 PS14, Line 329: Preconditions.checkState(tbl.isWriteLockedByCurrentThread(), Since you anyway check, why the caller has to send the boolean flag ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@354 PS14, Line 354: TODO: Can you write the final version of this TODO ? Are you going to do it in the same patch ? http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@474 PS14, Line 474: // TODO: should we ignore case? You might have to remove this TODO. http://gerrit.cloudera.org:8080/#/c/17859/14/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/17859/14/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@3060 PS14, Line 3060: String[] catAndDbName = MetaStoreUtils.parseDbName(dbNameWithCatalog, serverConf_); Why did you remove the try catch block ? http://gerrit.cloudera.org:8080/#/c/17859/14/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/17859/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@896 PS14, Line 896: * Updates the catalog db with alteredMsDb. To do so, first acquire lock on catalog db What is alteredMsDb ? Instead of saying "alteredMsDb", may be you have to say what it's meant for. http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/17859/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@421 PS14, Line 421: Remove all the unnecessary new lines -- To view, visit http://gerrit.cloudera.org:8080/17859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36364e401911352c4666674eb98c8d61bbaae9b9 Gerrit-Change-Number: 17859 Gerrit-PatchSet: 14 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: Tue, 05 Oct 2021 22:05:59 +0000 Gerrit-HasComments: Yes
