Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21065 )
Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event failures ...................................................................... Patch Set 11: (5 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: etTable( > Agreed. Not invalidating tables for create/drop/alter table rename events. Could you explain more why we don't invalidate tables for CreateTable event failures? I'm confused on the following questions: * Instead of triggering global invalidation (when --auto_global_invalidate_metadata is on), isn't trying invalidateTable() first and then global invalidation if it fails a cheaper solution? * When --auto_global_invalidate_metadata is off, we will directly go into the error states. Shouldn't we give invalidateTable() a try? http://gerrit.cloudera.org:8080/#/c/21065/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2761 PS3, Line 2761: The invalidati > Done I think if we handle failures of CreateTable events, we need invalidateTable() (instead of invalidateTableIfExists()) since the table doesn't exist in the catalog cache yet. invalidateTable() will bring up the table. invalidateTableIfExists() does nothing. But let's discuss whether to handle failures of CreateTable events first. http://gerrit.cloudera.org:8080/#/c/21065/11/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/21065/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1287 PS11, Line 1287: catalog_.invalidateTableIfExists(dbName_, tblName_); Let's trim the name strings in case of bugs like IMPALA-11939 http://gerrit.cloudera.org:8080/#/c/21065/11/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/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1012 PS11, Line 1012: LOG.error("Automatic global invalidate metadata failed", e); Shouldn't we re-throw the exception? http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py File tests/metadata/test_event_processing_error.py: http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@49 PS11, Line 49: "--inject_process_event_failure_event_types='ALTER_TABLE' " Currently, we inject the failure randomly at a possibility of 0.5. Can we make this configurable by a flag like 'inject_process_event_failure_ratio' and set it as 1? So this test can always produce failures in processing the ALTER_TABLE event. -- 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: 11 Gerrit-Owner: Anonymous Coward <[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: Tue, 05 Mar 2024 09:20:41 +0000 Gerrit-HasComments: Yes
