Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13748 )
Change subject: IMPALA-8571: harden QueryEventHook execution ...................................................................... Patch Set 28: (5 comments) Just a few replies to your useful comments. Waiting for next patch before doing more thinking http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG@19 PS28, Line 19: See Java Thread.setDaemon(boolean) for what a "daemon thread" entails. > I think it depends on whether or not they feel that query hook execution sh If I'm a user I don't want to read javadoc, I need help deciding. The flag is not even an advanced flag so sooner or later someone will have to explain the tradeoffs. This is the first actual change mentioned in the commit message so it must be important? http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java File fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java: http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@82 PS28, Line 82: * execute. You then create an executor with a thread pool of size 1 and a hook > Flashbacks to my technical-writing course :) :-) http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@182 PS28, Line 182: final ArrayBlockingQueue<Runnable> boundedQueue = > You mean declare the var as the most general type possible, right? Done. Yes, thanks http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@290 PS28, Line 290: t); > Yep, in fact it will log the stack trace this way! Does look weird and I al Thanks http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/service/Frontend.java@1624 PS28, Line 1624: * for {@link QueryEventHook}, which is part of the published api. > So the `QueryEventHook` class is published in a separate jar than the rest Thanks for thinking about this. I can see why the jar (and hence the api) is a separate artifact. My question is where the comments will be most useful. To me it seems like developers are most likely to be reading impala code than to be reading the api code. -- To view, visit http://gerrit.cloudera.org:8080/13748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb88422f7cfe86947d11ce57d2b4c63e57d1b643 Gerrit-Change-Number: 13748 Gerrit-PatchSet: 28 Gerrit-Owner: radford nguyen <radford.ngu...@gmail.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@apache.org> Gerrit-Reviewer: Fredy Wijaya <fre...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: radford nguyen <radford.ngu...@gmail.com> Gerrit-Comment-Date: Mon, 21 Oct 2019 16:34:40 +0000 Gerrit-HasComments: Yes
