Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Detect self-events before finishing DDL ...................................................................... Patch Set 16: (12 comments) http://gerrit.cloudera.org:8080/#/c/21029/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21029/14//COMMIT_MSG@48 PS14, Line 48: Testing: > Currently, we just have flaky tests like test_iceberg_self_events that can Added TestEventProcessingWithImpala. http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1203 PS14, Line 1203: : /** > Let's fix this as well, i.e. only log this when the version is added for th Done http://gerrit.cloudera.org:8080/#/c/21029/14/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/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1054 PS14, Line 1054: CatalogServiceCatalog catalog, Table table, long newVersionNumber) { > 'isInsertEvent' is always used as false. I think we can't track the infligh Done http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1080 PS14, Line 1080: * Cancel the new version number registration out of table's in-flight events. > nit: please add an error message. Done http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1221 PS14, Line 1221: : responseSum > nit: "Update the table object with the new catalog version" Done http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1248 PS14, Line 1248: : case DROP_PARTITION: : TAlterTableDropPartitionParams dropPartPa > nit: this comment is stale Removed. http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1376 PS14, Line 1376: > nit: maybe 'modification.isInProgress()' sounds better Done http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1592 PS14, Line 1592: if (!needsToUpdateHms) { > Can we remove this if the Iceberg operations won't update HMS? It also seem This is still needed, because ALTER_TABLE operation via Iceberg's HiveCatalog can produce event as well. What needsToUpdateHms means is whether caller of alterIcebergTable() need to follow up with separate HMS alter table call that is not through Iceberg library. http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2650 PS14, Line 2650: if (partition.getPartitionStatsCompressed() != null) { > This is unchanged yet. Changed to use InProgressTableModification. http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3239 PS14, Line 3239: try { > This is also unchanged but it's a bit complex - we are not always triggerin Changed all three truncate* function to return InProgressTableModification. This truncateTable() table only need to call markInflightEventRegistrationComplete(). http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5512 PS14, Line 5512: String.format(HMS_RPC_ERROR_FORMAT_STR, "alter_table"), e); > This is also unchanged for RENAME but I think we can't do it before the HMS Add TODO for future investigation. http://gerrit.cloudera.org:8080/#/c/21029/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7242 PS14, Line 7242: > This is also unchanged for inserting Iceberg tables. Changed to use InProgressTableModification. -- 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: 16 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 12 Mar 2024 12:58:18 +0000 Gerrit-HasComments: Yes
