Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15263 )
Change subject: IMPALA-9369: Make createInsertEvents() async. ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/15263/1/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/15263/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4315 PS1, Line 4315: avoid tables nit, may be change to "to avoid slowing down insert statements for large number of partitions" http://gerrit.cloudera.org:8080/#/c/15263/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4335 PS1, Line 4335: createInsertEventsAsync I think we should refactor the code here such that we only use copies of variables that we need for createInsertEvents method to work since we are potentially releasing the table lock before this method completes async now. http://gerrit.cloudera.org:8080/#/c/15263/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4339 PS1, Line 4339: files nit, number of partitions? http://gerrit.cloudera.org:8080/#/c/15263/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4340 PS1, Line 4340: table, affectedExistingPartitions, : isInsertOverwrite > Should we make local copies of these in this method since this method can t Most of my questions here are perhaps due to my limited knowledge of CompletableFuture but based on my reading of it, if we want to use this we should make sure that createInsertEvents is a pure function (no side-effect, no external state, repeatable). I am not sure if firing events in HMS is qualifies as no side-effect. Based on my understanding the thread from the executor will only be reclaimed when its garbage collected which could be a while. I think may be you should explicitly add a .thenRun(executor.shutdown()) as per here: https://stackoverflow.com/questions/35842910/how-do-i-shut-down-executor-when-using-it-with-completablefuture Also, unclear why do we need a complete(null) here. May be other reviewers who are more familiar with this should take a look too. -- To view, visit http://gerrit.cloudera.org:8080/15263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97802a5c03abc067fccf9e3a9d0047324626706e Gerrit-Change-Number: 15263 Gerrit-PatchSet: 1 Gerrit-Owner: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Fri, 21 Feb 2020 23:33:37 +0000 Gerrit-HasComments: Yes
