[email protected] 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 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/21065/8/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21065/8/be/src/catalog/catalog-server.cc@174 PS8, Line 174: DEFINE_string > nit: define this as hidden using DEFINE_string_hidden() Done 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( > I am not sure if it is really needed to deal with rename. Generally events Agreed. Not invalidating tables for create/drop/alter table rename events. http://gerrit.cloudera.org:8080/#/c/21065/7/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/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1292 PS7, Line 1292: e); : catalog_.invalidateTableIfExists(dbName_, tblName_); : } > nit: we can use warnLog() Done http://gerrit.cloudera.org:8080/#/c/21065/8/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/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@a2811 PS8, Line 2811: > Could you explain why we remove this? Have removed it from this gerrit. It is fixed with https://issues.apache.org/jira/browse/IMPALA-12851 now. http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@677 PS8, Line 677: BackendConfig.INSTANCE.getProcessEventFailureEventTypes() > nit: this can also be replaced with catalog_.getFailureEventsForTesting() We don't need this check anymore. Have directly call contains() instead. http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@679 PS8, Line 679: Double > nit: use primitive type 'double' Done http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1368 PS8, Line 1368: return false; > Maybe we can use CatalogServiceCatalog.invalidateDb() here. But it seems lo Have added a todo for now http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1787 PS8, Line 1787: tableBefore_.getDbName(), tableBefore_.getTableName()); > Please add a comment saying the new table will be invalidated in MetastoreT Not doing invalidateTableIfExists in case of rename table. Have added a comment for it now. http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2902 PS8, Line 2902: is not processed by then. So try to get the table again here if it is null */ > This seems fixing an existing bug. Can we split this into another patch? Done http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@3105 PS8, Line 3105: tableWriteId.getDbName(), tableWriteId.getTblName()); > I think we should deduplicate the table names before invalidating them. The Done http://gerrit.cloudera.org:8080/#/c/21065/8/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/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1006 PS8, Line 1006: try { > Can we add a log for this? Done http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1188 PS8, Line 1188: if (!event.onFailure(e)) { throw e; } > Can we add a log when throwing the exception? e.g. "Unable to handle failur Done http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1190 PS8, Line 1190: LOG.error(event.debugString("Failed to handle event processing failure")); > The invalidation in onFailure() could still fail by bugs like IMPALA-12831. Done -- 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: 8 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: Mon, 04 Mar 2024 13:25:29 +0000 Gerrit-HasComments: Yes
