Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/21663 )
Change subject: IMPALA-13126: Obtain table read lock in EP to process partitioned event ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/21663/6/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/21663/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1300 PS6, Line 1300: > Do we need to take the lock if partitionEventObj is null? Fair point, but if the partition is null then taking and releasing a lock should be very quick. But I'll go ahead with your suggestion. http://gerrit.cloudera.org:8080/#/c/21663/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1811 PS6, Line 1811: processRename(); : return; : } : : // Determine whether this is an event which we have already seen or if it is a new : // event : if (isSelfEvent()) { : infoLog("Not processing the event as it is a self-event"); : return; : } : // Ignore the event if this is a trivial event. See javadoc for : // canBeSkipped() for examples. : if (canBeSkipped()) { : metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); : infoLog("Not proces > The change could make the event processor "more blocking" by waiting for th With your above suggestion, I think it makes more sense to change in AlterPartition() event than here. http://gerrit.cloudera.org:8080/#/c/21663/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@3141 PS6, Line 3141: eturn false; } > Do we need to take the lock if reloadPartition_ is null? Ack http://gerrit.cloudera.org:8080/#/c/21663/6/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/21663/6/tests/custom_cluster/test_events_custom_configs.py@1334 PS6, Line 1334: > @pytest.mark.execute_serially could be added as this modifies a shared tabl Ack http://gerrit.cloudera.org:8080/#/c/21663/6/tests/custom_cluster/test_events_custom_configs.py@1348 PS6, Line 1348: se + ".test_in > Is it possible for the partition to exist at this point? The test could be No, not really. Since this is a shared table, and data is preloaded, this might bring in some flakiness if the partition is already present. So, I would prefer to play it safe for future tests. http://gerrit.cloudera.org:8080/#/c/21663/6/tests/custom_cluster/test_events_custom_configs.py@1347 PS6, Line 1347: acquiring write lock for CommitCompactionEvent""" : test_tbl = unique_database + ".test_invalidate_table" : acid_props = self._get_transactional_tblproperties(True) : self.client.execute("create table {} (id int) partitio > Can't these statements lead to throwing an exception? This would lead to no This will not lead to an exception until the reload event is processed at L#1353 -- To view, visit http://gerrit.cloudera.org:8080/21663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26933f98556736f66df986f9440ebb64be395bc1 Gerrit-Change-Number: 21663 Gerrit-PatchSet: 7 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 07 Jan 2025 13:40:07 +0000 Gerrit-HasComments: Yes
