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: (4 comments) 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@3608 PS14, Line 3608: if (catalog_.isExternalEventProcessingEnabled()) { > We don't wish to do this if event processing is disabled right? we can just collect it and discard if event processing is disabled. I think the extra branch affects readability, especially in this part of the code which is already too complex. 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()) { > We don't wish this path be taken if event processor is disabled. Right? same as above. 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); : } > As mentioned above this is not possible because of two different data struc Right. Why do we need to care about 'id' if the table is not partitioned. We will only have one partition (which is the only partition). That is what I meant when I said "using table.getNumClusteringCols() you'd know if filesBeforeInsert corresponds to a partitioned or a non partitioned table ". 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(); > This looks right to me. We want deltaFiles to be empty when it is an overwr "We want deltaFiles to be empty when it is an overwrite". Ok this is not obvious. Document this then? I thought that HMS expected that the delta should contain the new set of files even incase of an overwrite. -- 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: Sun, 14 Apr 2019 21:40:48 +0000 Gerrit-HasComments: Yes
