Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Deflake test_iceberg_self_events ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/21029/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21029/3//COMMIT_MSG@7 PS3, Line 7: Deflake test_iceberg_self_events I think that the title is a bit misleading, as this is an actual bug (though probably not correctness issue), not just a test problem. What about "Detect self-events before finishing DDL"? http://gerrit.cloudera.org:8080/#/c/21029/3//COMMIT_MSG@15 PS3, Line 15: IMPALA-12461 part1 Can you mention that currently this is only relevant for table level events, as self-event checking for partition level events still takes that table lock? http://gerrit.cloudera.org:8080/#/c/21029/3/fe/src/main/java/org/apache/impala/catalog/InflightEventParam.java File fe/src/main/java/org/apache/impala/catalog/InflightEventParam.java: http://gerrit.cloudera.org:8080/#/c/21029/3/fe/src/main/java/org/apache/impala/catalog/InflightEventParam.java@23 PS3, Line 23: InflightEventParam Just an idea: Table already has a concept of "InProgressModification" - currently it deals only with dirtyPartitions_ in case of HDFS table. It could be useful to have a class like InProgressTableModification that would help with tracking and cleaning up the dirty state of tables in CatalogOpExecutor. http://gerrit.cloudera.org:8080/#/c/21029/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/21029/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1120 PS3, Line 1120: inflightEventParam.addVersionsForInflightEvents(); Would it be possible to move this inside applyAlterTable()? My assumption is that applyAlterTable is called once in all cases in the switch where the HMS table object is modified, or not called if no modification is needed. Similarly, we should add the version as inflight event in case if there was an alter_table and shouldn't add it if it was not called, as in that case we'll never remove it from the in-flight event list. http://gerrit.cloudera.org:8080/#/c/21029/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1325 PS3, Line 1325: inflightEventParam.removeFromVersionsForInflightEvents(); Can you add some logging? This should be an unusual case, so it could be a warning. The result of removeFromVersionsForInflightEvents() is also interesting - if it returns false then the self-event was already consumed by the event processor. Knowing this could be useful when investigating HMS event issues. -- To view, visit http://gerrit.cloudera.org:8080/21029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79 Gerrit-Change-Number: 21029 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Mon, 19 Feb 2024 13:59:43 +0000 Gerrit-HasComments: Yes
