Riza Suminto 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 11: (8 comments) Thank you for keeping up with my 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()? http://gerrit.cloudera.org:8080/#/c/22997/11/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@323 PS11, Line 323: throw new EventProcessException(barrierEvent.getEvent(), e); In places where EventProcessException is thrown, please leave a comment on what will happen next. I think it will just trigger global IM and reset everything inside EventExecutorService? Is that why the code does not even bother to poll barrierEvents_ and do other cleanups? http://gerrit.cloudera.org:8080/#/c/22997/9/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java File fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java: http://gerrit.cloudera.org:8080/#/c/22997/9/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@441 PS9, Line 441: time to dispa > Done Done http://gerrit.cloudera.org:8080/#/c/22997/9/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@502 PS9, Line 502: long greatestSyncedEventId = p > Done Done http://gerrit.cloudera.org:8080/#/c/22997/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@628 PS9, Line 628: // Event creation time. It is initialized in constructor. It is not used for : // DerivedMetastoreEvent. : protected final long creationTime_; : : // Event dispatch time. It is initialized after dispatching event. It is not used for : // DerivedMetastoreEvent : protected long dispatchTime_; : : protected MetastoreEvent(CatalogOpExecutor catalogOpExecutor, Metrics metrics, : NotificationEvent event > Done Done 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 System.currentTimeMilis(). Thus, it is consistent regardless of type. Does this means DerivedMetastoreEvent will never reach addToInProgressLog()? 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? Global IM is expensive, and we'll want to minimize it. We should have metric to track how many time this happen. 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()? -- 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: 11 Gerrit-Owner: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Jun 2025 22:38:52 +0000 Gerrit-HasComments: Yes
