Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22554 )
Change subject: IMPALA-13593: Enable event processor to consume ALTER_PARTITIONS events from metastore ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/22554/7/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/22554/7/be/src/catalog/catalog-server.cc@223 PS7, Line 223: ALTER_PARTITIONS Is it intended to skip ALTER_PARTITION events after this patch? Are there scenarios that HMS still generate ALTER_PARTITION events, e.g. when the alter_partition HMS RPC is called? I think it'd be safer to keep both types. http://gerrit.cloudera.org:8080/#/c/22554/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/22554/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@634 PS7, Line 634: updatedFields.put("table", msTbl); : updatedFields.put("partitions", partitionsAfter); : updatedFields.put("isTruncate", isTruncateOp); nit: It might be better to define a class for this. Then we don't need cast in the call site. http://gerrit.cloudera.org:8080/#/c/22554/7/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/22554/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2860 PS7, Line 2860: "ALTER_PARTITIONS event" nit: use getEventDesc() http://gerrit.cloudera.org:8080/#/c/22554/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2866 PS7, Line 2866: } nit: L2843-2866 are quite similar to codes in AlterPartitionEvent#processTableEvent(). Can we extract them into a method to simplify the code? http://gerrit.cloudera.org:8080/#/c/22554/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2878 PS7, Line 2878: boolean incrementalRefresh = : BackendConfig.INSTANCE.getHMSEventIncrementalRefreshTransactionalTable(); : if (incrementalRefresh) { : reloadPartitionsFromEvent(partitionsAfter_, getEventDesc() : + " FOR TRANSACTIONAL TABLE"); : } else { : boolean notSkipped = reloadTableFromCatalog(true); : if (!notSkipped) { : metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); : } : } nit: These are also simlar to the implementation in AlterPartitionEvent. Can we extract them into a method to share them? http://gerrit.cloudera.org:8080/#/c/22554/3/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/22554/3/tests/custom_cluster/test_events_custom_configs.py@1671 PS3, Line 1671: EventProcessorUtils.wait_for_event_processing(self, 10) > Ack not done yet. http://gerrit.cloudera.org:8080/#/c/22554/3/tests/custom_cluster/test_events_custom_configs.py@1676 PS3, Line 1676: # Case-II: compute stats from impala > Ack not done yet. http://gerrit.cloudera.org:8080/#/c/22554/3/tests/custom_cluster/test_events_custom_configs.py@1679 PS3, Line 1679: events_skipped_before = EventProcessorUtils.get_int_metric('events-skipped', 0) > Ack not done yet. -- To view, visit http://gerrit.cloudera.org:8080/22554 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I009a87ef5e2c331272f9e2d7a6342cc860e64737 Gerrit-Change-Number: 22554 Gerrit-PatchSet: 7 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Tue, 08 Jul 2025 13:27:59 +0000 Gerrit-HasComments: Yes
