Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/13748 )
Change subject: IMPALA-8571: harden QueryEventHook execution ...................................................................... Patch Set 25: (9 comments) http://gerrit.cloudera.org:8080/#/c/13748/25//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13748/25//COMMIT_MSG@41 PS25, Line 41: but not yet : published to the backend and are therefore not user-visible I'm guessing this is done as a separate patch? http://gerrit.cloudera.org:8080/#/c/13748/25/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13748/25/be/src/service/impala-server.cc@285 PS25, Line 285: " nit: Prefix with "(Advanced)" like in other cases. We typically do not want the users to tinker with these. http://gerrit.cloudera.org:8080/#/c/13748/25/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/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@140 PS25, Line 140: oversightExecutor_ Call them timeoutMonitor_ or something? http://gerrit.cloudera.org:8080/#/c/13748/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@160 PS25, Line 160: FixedCapacityQueryHookExecutor(int nThreads, nit: 4sp indent http://gerrit.cloudera.org:8080/#/c/13748/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@169 PS25, Line 169: if (hookTimeout < 1) { : final String msg = String.format("hook timeout should be >= 1 but was {}. "); : LOG.error(msg); : throw new IllegalArgumentException(msg); : } nit: Preconditions.checkArgument(hookTimeout >=1, mesg); http://gerrit.cloudera.org:8080/#/c/13748/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@181 PS25, Line 181: nit: extra newline. http://gerrit.cloudera.org:8080/#/c/13748/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@190 PS25, Line 190: Ti nit: space http://gerrit.cloudera.org:8080/#/c/13748/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@304 PS25, Line 304: private QueryEventHook runOnQueryComplete( It looks like there is a 1:1 mapping between the monitoring thread pool and the executor thread pool. Say we configure the thread pool size to 16, we have 32 threads in total (at its peak). Instead, can't we have one thread that periodically wakes up (say every 1s), loops through all the running futures, checks if the elapsed time > timeout (we can store the start time for each of them and compute the delta with current time to get elapsed time) and interrupts it? Does that not work for some reason? Probably makes the code simpler and easier to reason too, lmk what you think. http://gerrit.cloudera.org:8080/#/c/13748/25/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/25/fe/src/main/java/org/apache/impala/hooks/QueryEventHookManager.java@248 PS25, Line 248: static final String ON_QUERY_COMPLETE = "onQueryComplete"; nit: Move to the top, static final consts are usually on the top. -- 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: 25 Gerrit-Owner: radford nguyen <radford.ngu...@gmail.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> 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: Wed, 25 Sep 2019 22:50:32 +0000 Gerrit-HasComments: Yes
