Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17308 )
Change subject: IMPALA-10502: Handle CREATE/DROP events correctly ...................................................................... Patch Set 6: (7 comments) LGTM, just have one question. I'm going to give my +1 once it's resolved. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@112 PS5, Line 112: throw new TableLoadingExceptio > The table has a new field createEventId which is used to track the event. W Done http://gerrit.cloudera.org:8080/#/c/17308/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/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1773 PS5, Line 1773: ((CreateDatabaseEvent) event).getDatabase()); : } catch (MetastoreNotificationException e) { : throw new CatalogException("Unable to create a metastore event ", e); : } : } : : /** > It was not too complex. I implemented it for create table and create databa Done http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3198 PS5, Line 3198: e > Yeah, I think we should change it to .equals() But perhaps in a separate ch Ack http://gerrit.cloudera.org:8080/#/c/17308/6/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/17308/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1764 PS6, Line 1764: useLatestEvent || events.size() == 1 nit: I think we should check events.size() > 0 (useLatestEvent && !events.isEmpty()) || events.size() == 1 http://gerrit.cloudera.org:8080/#/c/17308/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1796 PS6, Line 1796: useLatestEvent || events.size() == 1 nit: (useLatestEvent && !events.isEmpty()) || events.size() == 1 http://gerrit.cloudera.org:8080/#/c/17308/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4915 PS6, Line 4915: List<Partition> addedPartitions = addHmsPartitions(msClient, tbl, hmsPartitions, : partitionToEventId, true); Not sure about the latest hive changes, but is it safe to add all partitions in once now? Before this patch, we add at most 500 (MAX_PARTITION_UPDATES_PER_RPC) partitions in one RPC. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java@185 PS5, Line 185: "Fatal error while ini > Thanks for pointing this out. I think it is valid to throw CatalogException Done -- To view, visit http://gerrit.cloudera.org:8080/17308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c5e96b48abac015240f20295b3ec3b1d71f24a Gerrit-Change-Number: 17308 Gerrit-PatchSet: 6 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Mon, 24 May 2021 05:58:16 +0000 Gerrit-HasComments: Yes
