Quanlong Huang 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 19: (10 comments) http://gerrit.cloudera.org:8080/#/c/22997/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22997/17//COMMIT_MSG@24 PS17, Line 24: 4. Greatest synced event id is used in IMPALA-12152 changes. When : greatest synced event id becomes greater than or equal to : waitForEventId, all the required events are definitely synced. Just a note, as now table events are processed in parallel, we can improve the waitForEventSync feature to check table level lastSyncedEventIds for most of the scenarios. http://gerrit.cloudera.org:8080/#/c/22997/17/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/17/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@294 PS17, Line 294: currentTime nit: "startTime" might be better http://gerrit.cloudera.org:8080/#/c/22997/17/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/17/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@83 PS17, Line 83: Delimiter is a kind of metastore event that did not : // produce any pseudo-events. nit: can we give an example? E.g. add "e.g. INSERT events". http://gerrit.cloudera.org:8080/#/c/22997/17/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@88 PS17, Line 88: Boolean It seems moving this marker into MetastoreEvent as a new field is cleaner than using Pair. http://gerrit.cloudera.org:8080/#/c/22997/17/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@441 PS17, Line 441: ; nit: use event.getEventDesc() to also show the eventType. http://gerrit.cloudera.org:8080/#/c/22997/17/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@472 PS17, Line 472: nit: use value.getFirst().getEventDesc() to also show the eventType. http://gerrit.cloudera.org:8080/#/c/22997/18/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/18/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@472 PS18, Line 472: if (value == null) return; In which case will this happen? Could you add a comment about it? Do we need to do the work on L481-490? http://gerrit.cloudera.org:8080/#/c/22997/18/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@491 PS18, Line 491: Current dispatched event count nit: "dispatched event count" sounds like something that is monotonically increasing. Maybe it's clearer to say "Current count of dispatched events that are not in the processedLog". http://gerrit.cloudera.org:8080/#/c/22997/19/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/19/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@518 PS19, Line 518: Current processed event count nit: "processed event count" also sounds like something that is monotonically increasing. Maybe add "tracked" is more cleared, i.e. "Current count of processed events that are tracked". http://gerrit.cloudera.org:8080/#/c/22997/19/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java File fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java: http://gerrit.cloudera.org:8080/#/c/22997/19/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@805 PS19, Line 805: } Can we add test coverage on using addTo/removeFromInProgressLog() and pruneProcessedLog() in different scenarios? There are several branches in those methods that need test, E.g. assuming event 1 and 6 are non-delimiter events, add 1, 2, 3, 4, 5, 6 and then remove 3 and check greatestSyncedEventId. -- 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: 19 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: Mon, 08 Sep 2025 08:06:56 +0000 Gerrit-HasComments: Yes
