Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21031 )
Change subject: IMPALA-12709: Add support for hierarchical metastore event processing ...................................................................... Patch Set 45: (6 comments) http://gerrit.cloudera.org:8080/#/c/21031/45//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21031/45//COMMIT_MSG@104 PS45, Line 104: 1. We need to redefine the lag in the hierarchical processing mode. lastSyncedEventId is now used in the new feature of sync_hms_events_wait_time_s (IMPALA-12152) as well. Some test of it might fail when enable_hierarchical_event_processing=true. We can improve it based on the table level lastSyncedEventId. http://gerrit.cloudera.org:8080/#/c/21031/45/fe/src/main/java/org/apache/impala/catalog/events/DbBarrierEvent.java File fe/src/main/java/org/apache/impala/catalog/events/DbBarrierEvent.java: http://gerrit.cloudera.org:8080/#/c/21031/45/fe/src/main/java/org/apache/impala/catalog/events/DbBarrierEvent.java@76 PS45, Line 76: catalog_.getMetastoreEventProcessor().getDeleteEventLog().removeEvent(getEventId()); nit: Do we need this for all kinds of db events, or just DropDatabaseEvent needs this? It seems DeleteEventLog.removeEvent() is used in all kinds of events in this patch. It's not a correctness issue. Might just impact performance since removeEvent() is synchronized. http://gerrit.cloudera.org:8080/#/c/21031/45/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/21031/45/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@727 PS45, Line 727: process(); Now we have multiple threads processing different events. How about adding a thread annotation here? So in jstacks, we know the event id for each thread. try (ThreadNameAnnotator tna = new ThreadNameAnnotator( "Processing " + getEventDesc())) { process(); } http://gerrit.cloudera.org:8080/#/c/21031/45/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/21031/45/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@261 PS45, Line 261: public static final String OUTSTANDING_EVENT_COUNT = "outstanding-event-count"; nit: could you add a comment for this? Just like other constants. http://gerrit.cloudera.org:8080/#/c/21031/45/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/21031/45/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@58 PS45, Line 58: import org.junit.Assert; nit: unused import since all the used methods are imported above. http://gerrit.cloudera.org:8080/#/c/21031/45/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@248 PS45, Line 248: assertTrue(dbExecutor1.getTableEventExecutors().size() == 3); Many usages of assertTrue() in this patch can be replaced with assertEquals(long expected, long actual). Please use assertEquals() so when the test failed we have more info. -- To view, visit http://gerrit.cloudera.org:8080/21031 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76d8a739f9db6d40f01028bfd786a85d83f9e5d6 Gerrit-Change-Number: 21031 Gerrit-PatchSet: 45 Gerrit-Owner: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Anonymous Coward <cclive1...@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, 21 Apr 2025 14:40:17 +0000 Gerrit-HasComments: Yes
