k.venureddy2...@gmail.com 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 46: (10 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_ti Have updated the comment. Will fix it with follow-up. 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: actualEvent_.processIfEnabled(); > nit: Do we need this for all kinds of db events, or just DropDatabaseEvent Agreed. It is required only for delete events. Have modified it now. http://gerrit.cloudera.org:8080/#/c/21031/44/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/21031/44/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@171 PS44, Line 171: * There is no lock contention between the {@link DbEventExecutor#stop()} and : * {@link DbEventExecutor#proce > nit: I think processorLock_ and isTerminating_ term are more descriptive th Done http://gerrit.cloudera.org:8080/#/c/21031/44/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@340 PS44, Line 340: return isTerminating_; : } : } : : / > Taking example from IMPALA-13884, consider writing the stacktrace into debu Done http://gerrit.cloudera.org:8080/#/c/21031/44/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@396 PS44, Line 396: } : } : processDbEvents(); : skipE > This pattern is repeated in multiple places. I understand this serve as deb Done 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 Done. If we add ThreadNameAnnotator here, thread name gets duplicated when hierarchical processing is not enabled because we have already added ThreadNameAnnotator in org.apache.impala.catalog.events.MetastoreEventsProcessor#processEvents(long, java.util.List<NotificationEvent>). So have added ThreadNameAnnotator in DbEventExecutor and TableEventExecutor event 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: // time taken to fetch events during each poll > nit: could you add a comment for this? Just like other constants. Done http://gerrit.cloudera.org:8080/#/c/21031/44/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/21031/44/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@198 PS44, Line 198: * @return True if terminating. False otherwise : */ : private boolean isTerminating() { : synchronized (processorLock_) { : > Same suggestion as https://gerrit.cloudera.org/c/21031/42..44#344 Done 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.Before; > nit: unused import since all the used methods are imported above. Done http://gerrit.cloudera.org:8080/#/c/21031/45/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@248 PS45, Line 248: assertEquals(3, dbExecutor2.getTableEventExecutors().size()); > Many usages of assertTrue() in this patch can be replaced with assertEquals Done -- 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: 46 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: Sun, 27 Apr 2025 08:36:41 +0000 Gerrit-HasComments: Yes
