Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20148 )
Change subject: IMPALA-12257: Fix NPE in createInsertEvents when partitions only exist in HMS ...................................................................... Patch Set 6: (3 comments) Updated the test to cover transactional and overwrite scenarios http://gerrit.cloudera.org:8080/#/c/20148/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/20148/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6840 PS5, Line 6840: nsertEven > I am not sure about catching all Exceptions, as if there is am actual probl I'm not sure either. The behavior changes are in cases that createInsertEvents() fails but the following stuffs (e.g. commit transaction) all succeed. But I can't find out such cases yet. BTW, we also catch all exceptions in invoking HMS APIs in fireInsertEvents(): https://github.com/apache/impala/blob/bb7ad61c6c381ac25cd3fa198944230799fb8be8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java#L410-L412 So I feel like it's ok to skip exceptions here. http://gerrit.cloudera.org:8080/#/c/20148/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6973 PS5, Line 6973: insertEventInfo, table.getDb().getName(), table.getName()); : if (isTransactional) { : // ACID inserts do not generate INSERT events so there is nothing we need to track : // here for self-event detection. Note that It is enough to listen to the commit : // events fired by HMS, i.e. ALLOC_WRITE_I > Not related to the change, but this comment is stale now. I wrote this in I Nice catch! I see we currently don't detect self-events for our own commits, but skip them using the writeIds: https://github.com/apache/impala/blob/2a8374d7eb17592e6280b15f9adb046a25f2fb85/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L4827-L4828 BTW, I don't see the ALTER event when using Impala to insert the partition: When inserting into an existing partition of a transaction table, I see 3 events are generated: OPEN_TXN, ALLOC_WRITE_ID_EVENT, COMMIT_TXN. However, when using HiveServer to do the same thing, I see 4 events: OPEN_TXN, ALLOC_WRITE_ID_EVENT, ALTER_PARTITION, COMMIT_TXN. There is one more ALTER_PARTITION event. The ALTER_PARTITION event might be skipped as self-event which is a bug (IMPALA-12356). Updated the comment to explain the latest implementation. http://gerrit.cloudera.org:8080/#/c/20148/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7008 PS5, Line 7008: > Can you mention that this is not called for Hive ACID tables? Done -- To view, visit http://gerrit.cloudera.org:8080/20148 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d77844e26283ecb16b3b3aeb9f634bb3113eacd Gerrit-Change-Number: 20148 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 09 Aug 2023 23:44:55 +0000 Gerrit-HasComments: Yes
