Quanlong Huang 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: (3 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: > I think that this can still cause perf regression that could be avoided. +1 for avoid holding the lock when we don't need it. 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 > I am wandering if the old logic was actually buggy - my concern is that if That's a good point! I think the InFlightEvents list might leak some versions/eventIds due to this. It seems whenever we skip an event, we should check the InFlightEvents and update it in need. Unfortunately, modifying the InFlightEvents list requires holding the table lock again. Maybe an alternative is adding GC for InFlightEvents - when we witness an event id or catalog version from an event on the table, remove all eventIds/versions lower than that since they are stale. If we choose this way, it'd be better to do it in a separate patch. 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: > Ack Do we need this for a custom_cluster test? I think it restarts the Impala cluster and already runs serially. -- 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: Thu, 09 Jan 2025 01:48:49 +0000 Gerrit-HasComments: Yes
