Michael Smith 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 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/22554/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22554/2//COMMIT_MSG@10 PS2, Line 10: HIVE-27746 introduced ALTER_PARTITIONS event type which is an Please mention that we update our component versions to pick up this patch. http://gerrit.cloudera.org:8080/#/c/22554/2//COMMIT_MSG@24 PS2, Line 24: observed while looping the test several times. Is this general flakiness we see in other tickets? Why would implementing this potentially make event processing more variable? Or do you think they're related to the Hive version update? If it's just too slow for this particular test, we should specify the timeout for this test rather than changing the default. http://gerrit.cloudera.org:8080/#/c/22554/2/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/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@131 PS2, Line 131: ALTER_BATCH_PARTITIONS("ALTER_BATCH_PARTITIONS"), What is this? How is it related to the existing ALTER_PARTITIONS? http://gerrit.cloudera.org:8080/#/c/22554/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@133 PS2, Line 133: ALTER_PARTITIONS("ALTER_PARTITIONS"), This was already declared but unused? http://gerrit.cloudera.org:8080/#/c/22554/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2767 PS2, Line 2767: partitionsKeyVals_ = This never seems to be used. http://gerrit.cloudera.org:8080/#/c/22554/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2769 PS2, Line 2769: partitionsIterator_ = Preconditions.checkNotNull( Why is this a class property when we iterate through it immediately after and don't use it outside the constructor? http://gerrit.cloudera.org:8080/#/c/22554/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2788 PS2, Line 2788: infoLog("Not processing the alter partitions event {} as empty partitions are " + nit: "empty partitions" is ambiguous, it could be that there were no partitions or that each partition was empty. Maybe say "no partitions". http://gerrit.cloudera.org:8080/#/c/22554/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2797 PS2, Line 2797: if (isOlderEvent(partitionsAfter_.get(0))) { Could later partitions possibly be newer than the current state? That seems like it would be an error on HMS's end to send such a message. http://gerrit.cloudera.org:8080/#/c/22554/2/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/22554/2/tests/custom_cluster/test_events_custom_configs.py@1514 PS2, Line 1514: .format(transact_tbl, self._get_transactional_tblproperties(True))) There's different logic based on getHMSEventIncrementalRefreshTransactionalTable. Is that what this table is covering? This could be split into two test methods, using class-level CustomClusterTestSuite.with_args config to start a single cluster for both tests. -- 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: 2 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@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-Comment-Date: Wed, 16 Apr 2025 23:35:58 +0000 Gerrit-HasComments: Yes
