Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Detect self-events before finishing DDL ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/catalog/Table.java@1094 PS5, Line 1094: resetInProgressModification It look confusing to have resetInProgressTableModification and resetInProgressModification. I originally though about moving the old in progress modification to the new class, but it seems also good to change the name of the new function to include InFlighEvent, e.g. resetInProgressInFlighEvents() http://gerrit.cloudera.org:8080/#/c/21029/5/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/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1107 PS5, Line 1107: // If operation does not require metadata reload, in-progress table modification : // must be reset. It would be nice to simplify this. Could it work like first setting InflightEventParam, then applying it after the actual HMS RPC and instead of resetting simply store to a bool that it was applied? At the end of the function, reloadMetadata and this applied bool should be the same. http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1683 PS5, Line 1683: applyAlterTable(msTbl, true, null); > Original code never put in-flight event for ALTER VIEW. Please advise. I don't think that in-flight events are needed here. As for a view reloading the HMS object is a full reload, we can safely set setLastRefreshEventId below, which will lead to ignoring the event. Most alter table calls would need in-flight events as we don't reload the file metadata so the table is only "partially fresh", so lastRefreshEventId cannot be set. http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3807 PS5, Line 3807: applyAlterTable(msTable, true, null); > Original code never put in-flight event for createTable. Probably because a For "Original code" you mean the table creation with msClient.getHiveClient().createTable(newTable);? Self event checking is not needed for create table as we will ignore the event anyway as it already exists in the catalog: https://github.com/apache/impala/blob/2f14fd29c0b47fc2c170a7f0eb1cecaf6b9704f4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L843 http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5253 PS5, Line 5253: tbl.setCatalogVersion(tbl.getInProgressTableModification().versionNumber()); Is it necessary to increase the catalog version of the table? It DROP PARTITIONS looks like a NOOP in case of 0 partitions to drop. http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6368 PS5, Line 6368: MetastoreShim.alterTableWithTransaction(msClient.getHiveClient(), msTbl, tblTxn); : } else { : final InflightEventParam eventParam = Why are inflight events not handled in the transactional case? -- 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: 5 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: Thu, 22 Feb 2024 15:52:39 +0000 Gerrit-HasComments: Yes
