Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20022 )
Change subject: IMPALA-11535: Skip older events in the event processor based on the latestRefreshEventID ...................................................................... Patch Set 11: (8 comments) The solution looks good to me. Mainly focus on reviewing the tests. http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2939 PS11, Line 2939: partitions nit: let's also log the number of reloaded partitions, e.g. partBuilderToPartitions.size() Setting the latest refresh event id to 12345 for the reloaded 100 partitions http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3224 PS11, Line 3224: HdfsTable testTbl = (HdfsTable)catalog_.getOrLoadTable(TEST_DB_NAME, testTblName, : "test", null); : eventsProcessor_.processEvents(); These are done in testInsertEvents(). I think we don't need them. http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3233 PS11, Line 3233: Long nit: long http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3233 PS11, Line 3233: lastSyncEvenIdBefore typo: "Even" -> "Event" http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3235 PS11, Line 3235: assertTrue nit: use assertEquals() to have better error message http://gerrit.cloudera.org:8080/#/c/20022/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3246 PS11, Line 3246: } Can we test that REFRESH will also update LastRefreshEventId? E.g. do some Hive operations and run a REFRESH in Impala. Then process the events and verify they are all skipped. http://gerrit.cloudera.org:8080/#/c/20022/11/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/20022/11/tests/custom_cluster/test_events_custom_configs.py@324 PS11, Line 324: "create table {}.{} (i int) partitioned by (year int) " Can we also test transactional tables? http://gerrit.cloudera.org:8080/#/c/20022/11/tests/custom_cluster/test_events_custom_configs.py@345 PS11, Line 345: self.run_stmt_in_hive( : "insert into {db}.{tbl} partition (year=2024) values (10),(20),(30)" : .format(db=unique_database, tbl=test_old_table_1)) : self.run_stmt_in_hive( : "insert into {db}.{tbl} partition (year=2024) values (10),(20),(30)" : .format(db=unique_database, tbl=test_old_table_1)) : self.run_stmt_in_hive( : "insert into {db}.{tbl} partition (year=2024) values (10),(20),(30)" : .format(db=unique_database, tbl=test_old_table_1)) : self.client.execute( : "refresh {}.{}".format(unique_database, test_old_table_1)) Could you double check these can finish in 5s? If not, some events might be processed which makes this test flaky. We might need a longer hms_event_polling_interval_s -- To view, visit http://gerrit.cloudera.org:8080/20022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0dc5c7396d80616680d8a5805ce80db293b72e1 Gerrit-Change-Number: 20022 Gerrit-PatchSet: 11 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.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: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 22 Aug 2023 07:46:51 +0000 Gerrit-HasComments: Yes
