Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21065 )
Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata for table event failures ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/21065/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/21065/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2753 PS3, Line 2753: isRename I am not sure if it is really needed to deal with rename. Generally events that affect table existence (create,drop,rename) could be considered "global". It is hard to reason about what happens if one of these fails and later events would rely one the previous chain of create/drop/rename. http://gerrit.cloudera.org:8080/#/c/21065/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2761 PS3, Line 2761: invalidateTable Did you consider using invalidateTableIfExists() instead? It is not very intuitive, but invalidateTable() also checks the table's existence in Hive metastore and removes it if it doesn't exist. This means that invalidateTable() has more chance to fail and its effects can be more complex as it jumps to the current state in HMS. invalidateTable() is also complex from the aspect of thread safety as no lock is taken during the whole operation. To be honest I don't understand what would happen if there are parallel DDLs during invalidateTable() on the table. http://gerrit.cloudera.org:8080/#/c/21065/3/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/21065/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1170 PS3, Line 1170: if (((e instanceof RuntimeException : && !(e instanceof MetastoreClientInstantiationException)) : || (e instanceof MetastoreNotificationNeedsInvalidateException) : || (e instanceof CatalogException)) Can you add a comment that this also includes IllegalStateException from Preconditions (which is what we get in IMPALA-12827)? This was not evident for me. Also, moving this to a functions boolean exceptionNeedsGlobalInvalidate(Exception) would make it more readable. http://gerrit.cloudera.org:8080/#/c/21065/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1174 PS3, Line 1174: && (event.getTableName() != null)) { > Transaction events don't have the table name but might also be single-table I think that it would be more intuitive to handle this in a member functions of the events, e.g. boolean tryHandleErrorDuringProcessEvent() that returns false if invalidating the table is not enough and global invalidation is needed. Most subclasses of MetastoreTableEvent could handle it the same way while some event would need special handling or just return false. E.g. committxn/aborttxn would need to handle the possibility of multiple tables -- To view, visit http://gerrit.cloudera.org:8080/21065 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c Gerrit-Change-Number: 21065 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Sat, 24 Feb 2024 12:10:44 +0000 Gerrit-HasComments: Yes
