Sai Hemanth Gantasala 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:

(11 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.
Ack


http://gerrit.cloudera.org:8080/#/c/22554/2//COMMIT_MSG@12
PS2, Line 12: Impala
> Nit: in Impala.
Ack


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 t
I added bigger timeout specific to this test. This test is failing 2-3 times 
out of 100 because events were reaching slow to the EP, may be it has to do 
with my local env starting hive execution resources. I'll make it clear that I 
made timeouts bigger for this specific test.


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?
I want to use this for batch events and ALTER_PARTITIONS for the new event.


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?
Done


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.
Ack


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 a
Ack


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 partit
Ack


http://gerrit.cloudera.org:8080/#/c/22554/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2793
PS2, Line 2793: event
> Shouldn't we include the event id also here?
Event ID is embedded in the log message by default at L#837. Self-events are 
common occurrences in Log, so we try to avoid duplicate info.


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
All the partitions in ALTER_PARTITIONS event would have the same state, so it 
is enough to check one partition so see if they are older or newer events.


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 getHMSEventIncrementalRefreshTransactional
I want to cover the transactional and external tables scenario for the tests.

I didn't understand why we wanted to split the test into two different methods.



--
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 <[email protected]>
Gerrit-Reviewer: Daniel Becker <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]>
Gerrit-Comment-Date: Wed, 07 May 2025 20:06:22 +0000
Gerrit-HasComments: Yes

Reply via email to