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 14: (13 comments) http://gerrit.cloudera.org:8080/#/c/12889/13/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/12889/13/common/thrift/CatalogService.thrift@193 PS13, Line 193: // True if the update corresponds to an "insert overwrite" operation > nit: I think we should say "True if this update corresponds to an 'insert o Done http://gerrit.cloudera.org:8080/#/c/12889/13/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/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@396 PS13, Line 396: /** : * Util method to issue invalidate on a given table on the catalog. This method : * atomically invalidates the table if it exists in the catalog. No-op if the table : * does not exist : */ : p > don't think this needs a separate method. Inline it at the caller? Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@582 PS13, Line 582: > ..handler..Also, add some more color to it? Like it handles the inserts at Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@586 PS13, Line 586: : InsertEven > instead say. Null if the table is unprartitioned...or something like that? Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@614 PS13, Line 614: > this is obvious, remove? Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@619 PS13, Line 619: ** : * Process partition inserts : */ : private void processPartit > braces Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@630 PS13, Line 630: Preconditions.checkState(fsList.size() == partVals.size()); > Preconditions.checkNotNull(insertPartition_); Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@664 PS13, Line 664: d > unpartitioned .. Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@667 PS13, Line 667: try { > Preconditions.checkArgument(partition == null) Done http://gerrit.cloudera.org:8080/#/c/12889/13/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/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3583 PS13, Line 3583: Collection<? extends FeFsPartition> parts = : FeCatalogUtils.loadAllParti > Remove, this is obv? Done http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3589 PS13, Line 3589: List<FeFsPartition> existingPartitionsTouchedByInsert = new ArrayList<>(); > I suggest to refactor the code like this. Lemme know what you think. I thin Thanks for the suggestion. Sounds good. Refactored code according to your suggestion. However, tracking files for partitions is slightly different from that with table inserts. non-partitioned tables change ids after load, so we cannot track using a map. Hence the if...else for calculating files. http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3767 PS13, Line 3767: * fireInsertEvent() if external event processing is enabled. This is no-op otherwise. : * > shouldn't this be done only for the affected partitions? The filesBeforeInsertForPartitions map contains only the affected partitions. http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3823 PS13, Line 3823: * insert. In case of partitioned table, this event also contains the partition : * values of existing partitions which were touched by the insert. : */ : private void fireInsertEvent(Table tbl, List<String> partVals, : > insertData.setReplace(isOverwrite) 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: 14 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: Sat, 13 Apr 2019 02:14:27 +0000 Gerrit-HasComments: Yes
