[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 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/22997/6/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/6/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@269 PS6, Line 269: getEventProcessor(); > This is duplicated in multiple places. Consider making getter method for it Done http://gerrit.cloudera.org:8080/#/c/22997/6/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/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@81 PS6, Line 81: // In-progress event log. Maintains the metastore events waiting to be processed on : // executors. It is a map of metastore event id to the pair of metastore event and : // whether it is del > dispatchedEvents_ might be interpreted as finished/done. Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@86 PS6, Line 86: ng. The delimite > Consider renaming this to processedLog_. Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@178 PS6, Line 178: > Rename this to clearLogs? Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@449 PS6, Line 449: // Reinitialize with the current > This seems the only place setRecordTime() is called. We use the same field to determine the time taken for event dispatch and event process. Renaming it to dispatchedTime may cause confusion. It is first initialized/recorded when metastore event object is created(in the constructor). Time difference since the metastore event object is created until event is added to inProgressLog_ in EventExecutorService#addToInProgressLog() is the event dispatch time. Event dispatch time is logged in at line 443. And it is reinitialized with the current time to record the start time of the event processing, after logging the event dispatch time. Later it is used to log event process time, when event is removed from inProgressLog_ in EventExecutorService#removeFromInProgressLog(). what is your opinion ? http://gerrit.cloudera.org:8080/#/c/22997/6/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/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@637 PS6, Line 637: protected long recordTime_; > Consider renaming it to dispatchedTime_. We use the same field to determine the time taken for event dispatch and event process. Renaming it to dispatchedTime_ may cause confusion. It is first initialized/recorded when metastore event object is created(in the constructor). Time difference since the metastore event object is created until event is added to inProgressLog_ in EventExecutorService#addToInProgressLog() is the event dispatch time. Event dispatch time is logged in EventExecutorService#addToInProgressLog() at line 443. And it is reinitialized with the current time to record the start time of the event processing,after logging the event dispatch time. Later it is used to log event process time, when event is removed from inProgressLog_ in EventExecutorService#removeFromInProgressLog(). What is your opinion? http://gerrit.cloudera.org:8080/#/c/22997/6/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/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@179 PS6, Line 179: > This is duplicated in multiple places. Consider making getter method for it Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@249 PS6, Line 249: e void postProcessEvent(MetastoreEve > Should this be a Precondition? Looks like event is always MetastoreTableEve We can have DbBarrierEvent that need to ignored. Have refactored now such that we return immediately if it is DbBarrierEvent to avoid the confusion. Let me know if this looks better. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@472 PS6, Line 472: for (Map.Entry<String, TableProcessor> entry : tableProcessors_.entrySet()) { : TableProcessor tableProcessor = entry.getValue(); : if (!eventProcessor_.canProcessEventInCurrentStatus()) break; : > Can you create EventProcessException class and embed the failing event into Done -- 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: 9 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: Tue, 24 Jun 2025 15:02:24 +0000 Gerrit-HasComments: Yes
