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 13: (12 comments) I think we could still refactor the code better. I've added some suggestions, lemme know. I'll take another pass after the refactor. 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 to get partition key values of a given map of partition keys and values : */ : protected String getPartitionKeyValuesAsString(Map<String, String> partSpec) { : return Joiner.on(",").withKeyValueSeparator("=").join(partSpec); : } don't think this needs a separate method. Inline it at the caller? 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 the table and partition scope..... http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@586 PS13, Line 586: this is not a partition : // insert. instead say. Null if the table is unprartitioned...or something like that? http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@614 PS13, Line 614: This means insert events generated by impala will also be processed this is obvious, remove? http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@619 PS13, Line 619: if (insertPartition_ != null) : processPartitionInserts(); : else : processTableInserts(); braces http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@630 PS13, Line 630: Map<String, String> partSpec = new HashMap<>(); Preconditions.checkNotNull(insertPartition_); http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@664 PS13, Line 664: unpartitioned .. http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@667 PS13, Line 667: // For non-partitioned tables, refresh the whole table. Preconditions.checkArgument(partition == null) 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: // For non-partitioned tables, parts below will contain a single value. The : // partition key will be empty. Remove, this is obv? http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3589 PS13, Line 3589: Map<Long, Set<String>> filesBeforeInsertForPartitions = new HashMap<>(); I suggest to refactor the code like this. Lemme know what you think. I think that will be much cleaner and keeps the event code together than mixing it with the insert code. Also, I think for "overwrite" you don't need to compute a delta, which means you don't need to compute the fileListing "before" the load. dirtyParts = new List<>; if (partitioned) { for each partition affected in the logic below; dirtyParts.append(affectedPart) } else { dirtyParts.append(onlyPartition) } .... if (eventProcessingEnabled || !overwrite) { currentFileListing = getFilesForDirtyParts(); } loadMetadata(); if (eventProcessingEnabled) { fireEvents(fileListingBeforeLoad, dirtyParts()); } fireEvents() { computeDelta(); fireEvents(); } http://gerrit.cloudera.org:8080/#/c/12889/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3767 PS13, Line 3767: Collection<? extends FeFsPartition> partsPostInsert = : ((FeFsTable)table).loadPartitions(filesBeforeInsertForPartitions.keySet()); shouldn't this be done only for 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: if (isOverwrite) { : insertData.setReplace(true); : } else { : insertData.setReplace(false); : } insertData.setReplace(isOverwrite) -- 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: 13 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 00:46:29 +0000 Gerrit-HasComments: Yes
