Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12889 )
Change subject: IMPALA-7971: Add support for insert events in event processor. ...................................................................... Patch Set 6: (19 comments) http://gerrit.cloudera.org:8080/#/c/12889/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12889/5//COMMIT_MSG@14 PS5, Line 14: partitions. : : Known Issues: > This may not be applicable anymore Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@413 PS5, Line 413: ts(dbName_, tb > I think this call is not correct since this will be a no-op if the table is Thanks for this catch. Used reloadTable() instead which forces a reload every time. reloadPartition() http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@601 PS5, Line 601: * Metastore event for INSERT events. > These following two lines can be Done http://gerrit.cloudera.org:8080/#/c/12889/5/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/12889/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@204 PS5, Line 204: > Typo sofar Done http://gerrit.cloudera.org:8080/#/c/12889/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/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@38 PS5, Line 38: import org.apache.hadoop.hive.metastore.PartitionDropOptions; > unused? Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3551 PS5, Line 3551: // partition key will be empty. > nit : comma after tables. Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3557 PS5, Line 3557: InsertForTable = new HashSet<>(); > may be a better name would be to suggest that this contains files before in Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3558 PS5, Line 3558: eConf = new HiveConf(this. > is this unused? Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3580 PS5, Line 3580: rtition.isMarkedCached()) > Is there a concern here of running into CastException? I see that FeFsParti This code path is taken by only HDFSTables as you can see on L3522. http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3673 PS5, Line 3673: // For > may be do a else if(catalog.isExternalEventProcessingEnabled()) here so tha Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3674 PS5, Line 3674: Preconditions.checkState(parts.size() == 1); > May be add a Preconditions.checkState(parts.size == 1); here to make sure t Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3676 PS5, Line 3676: BeforeInsertForTable = ((( > same as above, Do we need to handle LocalFsPartition as well? The code path is taken by HdfsTable, so we do not need to handle LocalFsPartition. http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3699 PS5, Line 3699: createInsertEve > May be you can create 2 methods, one for partitioned case and another for n Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3716 PS5, Line 3716: } > Add to the description that this method is a no-op if event processing is d Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3719 PS5, Line 3719: nsert and ca > nit, change the name to isInsertOverwrite Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3736 PS5, Line 3736: List<String> newFiles = new ArrayList<>(); > I think it would be helpful to add info log here which says how many new fi Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3750 PS5, Line 3750: } > add a info level log here which prints how many new files were added into t Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3766 PS5, Line 3766: deltaFiles.removeAll(filesBeforeInsertForTable); > LOG.debug("Firing insert event for {}", tbl.getName()); Done http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3780 PS5, Line 3780: */ > I think it's better to have: Done -- To view, visit http://gerrit.cloudera.org:8080/12889 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c48c5ca4bde18d532c582980aebbc25f1bf1c52 Gerrit-Change-Number: 12889 Gerrit-PatchSet: 6 Gerrit-Owner: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Mon, 08 Apr 2019 23:05:17 +0000 Gerrit-HasComments: Yes
