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 9: (8 comments) 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 {} events. First event id: {}.", response.getEvents().size(), : (response.getEvents().size() > 0 ? response.getEvents().get(0).getEventId() : : "none")); : if (filter > nit: It'd be more readable to use the message template: Done 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: if (msTbl.getPartitionKeysSize() == > +1. Could you add a comment for this? Done 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 t Done http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1111 PS7, Line 1111: */ : public void validateInProgressModificationComplete() { > nit: please add error messages for these Done http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1366 PS7, Line 1366: } > nit: let's add an error message for Preconditions. Done http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4493 PS7, Line 4493: } > Might be an existing issue, but should we still do this when 'addedHmsParti Probably not, but I prefer matching the old-behavior like in old-code, L1147. We can investigate the impact of not doing this (and another seemingly unnecessary code in L5307) in separate patch and see which test will break. http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5819 PS7, Line 5819: KuduCatalogOpExecutor.validateKuduTblExists(msTbl); > Just realized this is invoked in alterTable() instead of alterView().. In a This probably need to be handled by the switch anyway for SET_VIEW_PROPERTIES enum value. http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6135 PS7, Line 6135: } > Should we do this before L6106 where the events will be generated, and hand This is one spot where I'm no sure about. What if some alterPartitions went through and the rest is not? Fortunately, I think partition-level events handling still acquire table locks. >From IMPALA-12461 commit message: "Postponing solving this for partition level events as that would be more complex, as both the partition list and the partitions' in-flight event lists would need to be protected from parallel operations that add/remove partitions." So cancelInflightEvent is not required here, just like other alter partition operation (ie., alterTableDropPartition and alterTableAddPartitions). -- 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: 9 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 06:36:59 +0000 Gerrit-HasComments: Yes
