Bharath Vissapragada 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: (9 comments) Some more suggestions. I think it is getting closer. http://gerrit.cloudera.org:8080/#/c/12889/14/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/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3587 PS14, Line 3587: Map<Long, Set<String>> filesBeforeInsertForPartitions = new HashMap<>(); : Set<String> filesBeforeInsertForTable = new HashSet<>(); Both of them can be handled in a single map? Also, rename to filesBeforeInsert? http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3589 PS14, Line 3589: existingPartitionsTouchedByInsert just say affectedPartitions ? http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3608 PS14, Line 3608: if (catalog_.isExternalEventProcessingEnabled()) { not needed. http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3704 PS14, Line 3704: if (catalog_.isExternalEventProcessingEnabled()) { not needed. http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3736 PS14, Line 3736: filesBeforeInsertForTable = ((HdfsPartition) Iterables Like I mentioned above, I don't see why we should use two different datastructures here. Even an unparitioned table is implemented as a single partitioned table. I suggest to refactor like this. if (eventProcessingEnabled && !overwrite) .{ for (part: affectedPartitions) { filesBeforeInsert.put(part.id, part.getFiles()); } } http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3743 PS14, Line 3743: // After loading metadata, fire insert event if external event processing is : // enabled. : if (table.getNumClusteringCols() > 0) { : createInsertEventsForPartitions(table, filesBeforeInsertForPartitions, : update.is_overwrite); : } else { : createInsertEventForTable(table, filesBeforeInsertForTable, update.is_overwrite); : } How about changing the signature to fireInsertEvents(table, filsBeforeInsert, is_overwrite) using table.getNumClusteringCols() you'd know if filesBeforeInsert corresponds to a partitioned or a non partitioned table (you could create your event accordingly) http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3787 PS14, Line 3787: deltaFiles = ((HdfsPartition)part).getFileNames(); shouldn't this go before if? tests didn't catch this? http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3791 PS14, Line 3791: String.valueOf( need this? http://gerrit.cloudera.org:8080/#/c/12889/14/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3813 PS14, Line 3813: deltaFiles = ((HdfsPartition)singlePart).getFileNames(); same question, shouldn't deltaFiles go before if? deltaFiles = allCurrentFiles; if (!overrwrite) { deltaFiles.removeAll(oldFiles) } -- 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 03:08:06 +0000 Gerrit-HasComments: Yes
