Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21065 )
Change subject: IMPALA-12832: Implicit invalidate metadata on event failures ...................................................................... Patch Set 21: Code-Review+1 (8 comments) Left a few comments, mainly about logging. I think that we should log event processing errors at ERROR level, even if it was handled with invalidate. Otherwise lgtm http://gerrit.cloudera.org:8080/#/c/21065/21/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21065/21/be/src/catalog/catalog-server.cc@190 PS21, Line 190: Set this value to ~0.0 to fail the EP consistently This seems stale. http://gerrit.cloudera.org:8080/#/c/21065/21/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/21065/21/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@887 PS21, Line 887: warnLog This should be logged at error level. http://gerrit.cloudera.org:8080/#/c/21065/16/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/16/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@677 PS16, Line 677: > Ack +1 for Quanlong's comment, < looks safer than <= http://gerrit.cloudera.org:8080/#/c/21065/21/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/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@685 PS21, Line 685: Event process failed Could be more descriptive, e.g. "Event processing failed due to error injection. It could be also useful to move this to a separate function, e.g. InjectErrorIfNeeded() - this would mean that the callstack will make it clear that this error was injected on purpose. http://gerrit.cloudera.org:8080/#/c/21065/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1298 PS21, Line 1298: warnLog This should be logged at error level. http://gerrit.cloudera.org:8080/#/c/21065/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1802 PS21, Line 1802: warnLog This should be logged at error level. http://gerrit.cloudera.org:8080/#/c/21065/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@3105 PS21, Line 3105: warnLog This should be logged at error level. http://gerrit.cloudera.org:8080/#/c/21065/21/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/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1189 PS21, Line 1189: event.warnLog("Unable to handle event processing failure"); Probably this should be logged at error level? -- 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: 21 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: Thu, 07 Mar 2024 07:42:15 +0000 Gerrit-HasComments: Yes
