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 46: (10 comments) 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(); > Done. If we add ThreadNameAnnotator here, thread name gets duplicated when Ack http://gerrit.cloudera.org:8080/#/c/21031/46/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/46/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@130 PS46, Line 130: try( nit: need one space http://gerrit.cloudera.org:8080/#/c/21031/46/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@228 PS46, Line 228: assertEquals(0, metastoreEvents.size()); nit: Add an error message in case this fails assertEquals("Remaining events: " + metastoreEvents, 0, metastoreEvents.size()); http://gerrit.cloudera.org:8080/#/c/21031/46/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@300 PS46, Line 300: * nit: add spaces http://gerrit.cloudera.org:8080/#/c/21031/46/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@311 PS46, Line 311: assertEquals(null, eventExecutorService.getDbEventExecutor(DB_NAME1)); nit: use assertNull() directly http://gerrit.cloudera.org:8080/#/c/21031/46/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@681 PS46, Line 681: AtomicInteger counter = new AtomicInteger(); nit: please rename or comment this about what it counts for. http://gerrit.cloudera.org:8080/#/c/21031/46/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@692 PS46, Line 692: while (!state.isDropProcessed()); nit: 'while' statement has empty body. Add a sleep? http://gerrit.cloudera.org:8080/#/c/21031/46/tests/custom_cluster/test_event_processing_perf.py File tests/custom_cluster/test_event_processing_perf.py: http://gerrit.cloudera.org:8080/#/c/21031/46/tests/custom_cluster/test_event_processing_perf.py@340 PS46, Line 340: create_db_table_time = self.__process_events(start_event_id) nit: It'd be helpful to add a new mode that doesn't synchronize the processing of different kinds of events. So we can see the overall duration and test processing different kinds of events in parallel. http://gerrit.cloudera.org:8080/#/c/21031/46/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/21031/46/tests/custom_cluster/test_events_custom_configs.py@1512 PS46, Line 1512: hep nit: what's the meaning of "hep" here? typo? http://gerrit.cloudera.org:8080/#/c/21031/46/tests/custom_cluster/test_events_custom_configs.py@1592 PS46, Line 1592: I might have missed it but do we have test coverage on BarrierEvent blocks other events on the table? E.g. assuming t1 and t2 are loaded tables in Impala and run these in Hive: drop table if exists t2; alter table t1 rename to t2; insert into t2; create table t1 as select * from t2; Processing events generated by these should all succeed and tables should be up-to-date at the end. -- 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: Mon, 28 Apr 2025 03:54:22 +0000 Gerrit-HasComments: Yes
