Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20367 )
Change subject: IMPALA-10976: Sync db/table in catalogD to latest HMS event id for all DDLS from Impala clients ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/20367/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20367/4//COMMIT_MSG@8 PS4, Line 8: for all DDLS Can you add to the title whether Impala syncs the events before or after the DDL operation? I think that this a critical info about the intention of the patch http://gerrit.cloudera.org:8080/#/c/20367/4//COMMIT_MSG@20 PS4, Line 20: Once HIVE-27499 is implemented, we can update the : snapshot only if there are any pending events. Currently, there is no : efficient way to identify if there are pending events for a db/table. Won't there be always a pending event, as the DDL from Impala creates a new event for the DB/Table? http://gerrit.cloudera.org:8080/#/c/20367/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/20367/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1396 PS3, Line 1396: versionLock_.writeLock().lock(); > We always obtain a write lock on the table in CatalogOpExecutor before doin I think that we also need to hold versionLock_ before calling this, as the locking protocol states that version lock should be acquired before table lock. An example where we take version lock first and then the table lock is CatalogServiceCatalog.getOrLoadTable Note that this is probably irrelevant, see comment at line 1391. http://gerrit.cloudera.org:8080/#/c/20367/4/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/20367/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1391 PS4, Line 1391: updateMetastoreTable I couldn't find any place where we call this function - is it still used? http://gerrit.cloudera.org:8080/#/c/20367/4/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/20367/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1546 PS4, Line 1546: alterTableRename(testTblName, newTblName, TEST_DB_NAME); : eventsProcessor_.processEvents(); What will happen if a rename is detected during a DDL and not in eventsProcessor_.processEvents();? E.g. a t1 is renamed to t2, then back to t1, and Impala tries to do a DDL on t1. I don't think that we really need a proper handling for this, but it shouldn't cause global issues in the catalog. -- To view, visit http://gerrit.cloudera.org:8080/20367 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia250d0a943838086c187e5cb7c60035e5a564bbf Gerrit-Change-Number: 20367 Gerrit-PatchSet: 4 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Fri, 27 Oct 2023 16:14:25 +0000 Gerrit-HasComments: Yes
