[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/22997 )
Change subject: IMPALA-13801: Support greatest synced event with hierarchical metastore event processing ...................................................................... Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/22997/11/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/22997/11/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@317 PS11, Line 317: synchronized (processorLock_) { : if (isTerminating()) return; : Preconditions.checkState(barrierEvents_.poll() == barrierEvent); : dbEventExecutor_.decrOutstandingEventCount(1); : } > Should this moved into postProcessEvent()? postProcessEvent(event) does the post operations related to the particular event processed. IMO it is good to keep it here in the while loop. Also this looping and polling of barrierEvents_ in the same method process() avoid confusion too. Same for TableProcessor too. http://gerrit.cloudera.org:8080/#/c/22997/11/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@323 PS11, Line 323: // Throwing EventProcessException triggers global invalidate > In places where EventProcessException is thrown, please leave a comment on Done. Yes, it requires invalidate metadata and that resets everything inside EventExecutorService, DbEventExecutor and TableEventExecutor. When invalidate_global_metadata_on_event_processing_failure flag is configured as true, invalidate metadata happens without user intervention. http://gerrit.cloudera.org:8080/#/c/22997/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/22997/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@628 PS11, Line 628: It is not used for : // DerivedMetastoreEvent. > Even if it is not used, it probably does not hurt to initialize it to Syste Yes only actual events are added to addToInProgressLog() after queuing actual event or derived event(if any). So time to dispatch and time to process is time taken for actual event or all of its derived event(if any). Initialized creationTime_ irrespective of actual or derived metastore event. It is of no use in case of derived events. So have retained comment that it is not used for DerivedMetastoreEvent. http://gerrit.cloudera.org:8080/#/c/22997/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/22997/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1397 PS11, Line 1397: dumpEventInfoToLog(event, LocalDateTime.now().toString() + '\n' + msg + : '\n' + ExceptionUtils.getStackTrace(ex)); : tryAutoGlobalInvalidateOnFailure(); > Have you check inside CatalogD logs if this ever happen? If yes, how often? It is not expected to happen in normal operation. It can happen if there is some exception in event processing may be due to Preconditions failure and MetastoreEvent#onFailure(Exception e) returns false. Auto global invalidation is controlled with invalidate_global_metadata_on_event_processing_failure and it is false by default. So if it hits this code, user need to invalidate manually assuming invalidate_global_metadata_on_event_processing_failure has default value. There is a mechanism added with IMPALA-12832 to simulate the failures. And tests are added with it. tests/custom_cluster/test_event_processing_error.py::TestEventProcessingError http://gerrit.cloudera.org:8080/#/c/22997/11/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/22997/11/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@332 PS11, Line 332: synchronized (processorLock_) { : if (isTerminating()) return; : Preconditions.checkState(events_.poll() == event); : tableEventExecutor_.decrOutstandingEventCount(1); : } > Should this moved into postProcessEvent()? postProcessEvent(event) does the post operations related to the particular event processed. IMO it is good to keep it here in the while loop. Also this looping and polling of events_ in the same method process() avoids confusion too. -- To view, visit http://gerrit.cloudera.org:8080/22997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26240f36aaf85125428dc39a66a2a1e4d3197e85 Gerrit-Change-Number: 22997 Gerrit-PatchSet: 12 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: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Thu, 26 Jun 2025 18:58:43 +0000 Gerrit-HasComments: Yes
