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 16: (5 comments) lgtm, can +2 once the remaining nits are fixed. http://gerrit.cloudera.org:8080/#/c/12889/16/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/16/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3587 PS16, Line 3587: Map<Long, Set<String>> filesBeforeInsert = new HashMap<>(); nit: Move closer to it's usage. http://gerrit.cloudera.org:8080/#/c/12889/16/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3723 PS16, Line 3723: // Get the files before insert. This is used to compute the delta of files to : // fire insert events. : if (catalog_.isExternalEventProcessingEnabled()) { : if (!update.is_overwrite) { : for (FeFsPartition partition : affectedExistingPartitions) { : filesBeforeInsert.put(partition.getId(), : ((HdfsPartition) partition).getFileNames()); : } : } : } nit: move this inside createInsertEvents()? http://gerrit.cloudera.org:8080/#/c/12889/16/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3773 PS16, Line 3773: This list is empty if this is an insert overwrite operation. nit: May be rephrase to what HMS expects? That way semantics are more clear. http://gerrit.cloudera.org:8080/#/c/12889/16/tests/custom_cluster/test_event_processing.py File tests/custom_cluster/test_event_processing.py: http://gerrit.cloudera.org:8080/#/c/12889/16/tests/custom_cluster/test_event_processing.py@59 PS16, Line 59: time.sleep(10) Is there a more deterministic way of doing this? Might turn out to be a flaky test in builds like ASAN. http://gerrit.cloudera.org:8080/#/c/12889/16/tests/custom_cluster/test_event_processing.py@79 PS16, Line 79: time.sleep(10) same. -- 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: 16 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, 15 Apr 2019 17:30:57 +0000 Gerrit-HasComments: Yes
