Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22997 )
Change subject: IMPALA-13801: Support guaranteed minimum synced event with hierarchical metastore event processing ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/22997/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22997/2//COMMIT_MSG@18 PS2, Line 18: Guaranteed minimum synced event id Please elaborate more what is "Guaranteed minimum synced event id" means. Maybe "Greatest synced event id" is a better term here, with following definition: Greatest synced event id is the latest event id X such that all event id <= X has been completely processed. http://gerrit.cloudera.org:8080/#/c/22997/2/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/2/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@489 PS2, Line 489: getGuaranteedMinSyncedEventId nit: getGreatestSyncedEventId sounds like a better terminology. http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@504 PS2, Line 504: getGuaranteedMinSyncedEventTime nit: getGreatestSyncedEventDispatchTime sounds like a better terminology. http://gerrit.cloudera.org:8080/#/c/22997/2/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/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@628 PS2, Line 628: // To record the current time in milliseconds. It can be used to calculate the time : // taken for the particular operation like, event dispatch time, processing : // time etc. : protected long recordTime_; When is recordTime_ intialized? During dispatch? If yes, I think dispatchTime_ is more specific. If there are other point of time to record, please consider creating separate filed for each. http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1015 PS2, Line 1015: processed markProcessed sounds better. http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1023 PS2, Line 1023: Abstract class for derived metastore table event. Please add to class documentation: A single-real MetastoreTableEvent can have multiple DerivedMetastoreTableEvent associated with it. http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1035 PS2, Line 1035: CatalogOpExecutor catalogOpExecutor, : Metrics metrics, NotificationEvent event, AtomicInteger derivedEventsCount nit: it is probably worth to wrap these params into single DerivedMetastoreEventContext class and keep this context object as class member of DerivedMetastoreTableEvent. But I don't mind the way it is now, since derivedEventsCount_ is the only shared state right now. http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1062 PS2, Line 1062: isActualEventProcessed isAllDerivedEventsProcessed sounds more straightforward. http://gerrit.cloudera.org:8080/#/c/22997/2/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/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1019 PS2, Line 1019: * Gracefully pauses the event processor by setting its status to : * <code>EventProcessorStatus.PAUSED</code>. Where is this set? I do not see pauseGracefully() propagate to pause() anywhere. -- 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: 2 Gerrit-Owner: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Tue, 10 Jun 2025 20:01:48 +0000 Gerrit-HasComments: Yes
