Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Detect self-events before finishing DDL ...................................................................... Patch Set 7: (8 comments) Thanks for fixing this! It's a great work that requires detailed reviews. Left some comments first. I'll look deeper into the patch. http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@909 PS7, Line 909: LOG.info("Received " + response.getEvents().size() + " events. First event id : " : + (response.getEvents().size() > 0 ? response.getEvents().get(0).getEventId() : : "none") : + "."); nit: It'd be more readable to use the message template: LOG.info("Received {} events. First event id: {}.", response.getEvents().size(), (response.getEvents().size() > 0 ? response.getEvents().get(0).getEventId() : "none")); 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@1683 PS5, Line 1683: > I don't think that in-flight events are needed here. As for a view reloadin +1. Could you add a comment for this? http://gerrit.cloudera.org:8080/#/c/21029/7/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/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1066 PS7, Line 1066: * Register the new version number into table's in-flight events. Could you add a comment for when should we use this, e.g. before invoking the HMS API for non-insert events? http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1111 PS7, Line 1111: Preconditions.checkState(!table_.hasInProgressModification()); : Preconditions.checkState(!inflightEventRegistered_); nit: please add error messages for these http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1366 PS7, Line 1366: Preconditions.checkState(reloadMetadata); nit: let's add an error message for Preconditions. http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4493 PS7, Line 4493: modification.registerInflightEvent(); Might be an existing issue, but should we still do this when 'addedHmsPartitions' is empty, i.e. no partitions are added? http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5819 PS7, Line 5819: private void alterViewSetTblProperties(Table tbl, Just realized this is invoked in alterTable() instead of alterView().. In alterView(), we don't deal with the inflight versions (at L1754). Maybe we should make them consistent. But not a big deal since reload on views should be fast, unless it's a materialized view (IIRC, we can't alter MV yet). http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6135 PS7, Line 6135: modification.registerInflightEvent(); Should we do this before L6106 where the events will be generated, and handle the failure at L6128? -- 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: 7 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: Tue, 05 Mar 2024 02:05:38 +0000 Gerrit-HasComments: Yes
