Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13748 )
Change subject: IMPALA-8571: harden QueryEventHook execution ...................................................................... Patch Set 28: (25 comments) Hi Radford. As Bharath and Fredy have disappeared I'll review this now :-) Code looks good. Like you I'm slightly worried about the timeout queue. It's somehow wasteful to keep all that state around when the common case is that the tasks completed quickly. 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. How can a user decide whether to use daemon threads? http://gerrit.cloudera.org:8080/#/c/13748/28//COMMIT_MSG@24 PS28, Line 24: flag `query_event_hook_timeout_s`, which specified a timeout value Nit: specifies 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@47 PS28, Line 47: * {@link QueryEventHook}s that utilizes a fixed thread pool pulling fixed capacity thread pool? http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@62 PS28, Line 62: * submission, you will have no indication that rejection has taken place. "there is no indication" i.e. passive voice may be clearer http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@81 PS28, Line 81: * For example, suppose you have 2 hook tasks that take roughly 3 seconds to This example is great, thanks 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 You can consider this a nit :-) , but I find the use of 'you' jarring 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 = Can you use BlockingQueue<Runnable> ? http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@290 PS28, Line 290: t); Does LOG.error work when there is an extra Throwable argument? Looks a bit weird to me http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@315 PS28, Line 315: // a timeout-check task is scheduled for every hook task that is Nit: capitalize 'a' http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@318 PS28, Line 318: // task queue to grow unbounded if hook tasks are scheduled at an interval This approach seems clever, and the code is neat and tidy... So suppose I have problems getting my hooks to always execute. I know they usually take 5 seconds but just in case I will set the timeout to 30 mins. Now my timeout queue gets quite full I think? How much garbage does the timeout queue keep from being collected? http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@327 PS28, Line 327: protected void checkHookTimeout( private http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@354 PS28, Line 354: private static <T> CompletableFuture<T> failedFuture(Throwable e) { Maybe explain what is intended to happen http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@365 PS28, Line 365: private static String mname(String... names) { Maybe mName is clearer? http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@370 PS28, Line 370: Counter getInterruptedCounter(QueryEventHook hook, String method) { Unused? http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java File fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java: http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@75 PS28, Line 75: // constants to share with the executor so that no mismatches Nit: Constants http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@78 PS28, Line 78: // it'd be nice if we could get the flag names from the be Maybe instead say "The must be kept in sync with..." http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@135 PS28, Line 135: * @param hookTimeout_s If there is no description here then delete the @params as they add no value. http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@150 PS28, Line 150: final String msg = String.format("# hook threads should be positive but was {}. " + I think {} is wrong here, use %d http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@157 PS28, Line 157: final String msg = String.format("hook timeout should be positive but was {}. " + I think {} is wrong here, use %d http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@164 PS28, Line 164: final String msg = String.format("hook queue capacity should be >= 0 but was {}. " + I think {} is wrong here, use %d 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. I would like to see the guarantees here. Having them in a separate place they may change or disappear completely http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java File fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java: http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@108 PS28, Line 108: private void assertOnQueryCompleteTimedOut(Future<QueryEventHook> hookFuture, Maybe assertHookTimedOut http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@119 PS28, Line 119: public void testFastHookCompletionDoesntTimeout() throws Exception { These tests all have good names but a one line description is always helpful http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@190 PS28, Line 190: // rejected or submission. rejected for submission http://gerrit.cloudera.org:8080/#/c/13748/28/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@197 PS28, Line 197: throw ee.getCause(); Nit: maybe simpler to just have an assertion here rather than adding complexity of expectedException ? -- 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: Thu, 17 Oct 2019 00:42:17 +0000 Gerrit-HasComments: Yes
