Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13748 )
Change subject: IMPALA-8571[WIP]: improve QueryEventHook execution ...................................................................... Patch Set 18: (11 comments) I think this going in the right direction http://gerrit.cloudera.org:8080/#/c/13748/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13748/18//COMMIT_MSG@31 PS18, Line 31: Do you want to discuss --query_event_hook_use_daemon_threads here? How would a user determine what value to use for that? http://gerrit.cloudera.org:8080/#/c/13748/18/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/18/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@51 PS18, Line 51: * <h3>Rejections</h3> This javadoc is great, and just the sort of thing I was hoping to see. Are you expecting to publish the html that is generated from this javadoc? If not then I think Impala code generally does not include the formatting stuff like <h3><p>. I think we generally emphasize readability in the editor. http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@102 PS18, Line 102: * <dt>${hookClass}.${method}.execution.exceptions</dt> I think the metrics now have the "query-event-hook" prefix. I'm also unclear about the exact meanings of hookClass and method. Detail in metrics is good, but so is predictability. If I can't predict the metrics name then it is harder to write tooling that uses it. I can see this both ways, what do you think? http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@176 PS18, Line 176: // ArrayBlockingQueue contructor performs bounds-check on queue size for us typo: constructor http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@184 PS18, Line 184: // this executor cancels any hook tasks that This may seem picky but in Imapla we like comments with Capitals at the beginning and periods at the end. http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@255 PS18, Line 255: * {@link ExecutionException}. This behavior is consistent with how futures normally We could be more concise by not saying this about how Futures normally behave, as that's what we expect, right? http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@286 PS18, Line 286: LOG.warn("QueryEventHook {}.{} execution rejected because the " + I sometimes weaselly write "probably because" as you never know :-) I know executor,getActiveCount() is only a snapshot but it might show something interesting. http://gerrit.cloudera.org:8080/#/c/13748/18/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/18/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@1 PS18, Line 1: /** Use // style comments for the apache license please http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@49 PS18, Line 49: new QueryCompleteContext("whatever"); lol http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@80 PS18, Line 80: // make this longer to reasonably guarantee a timeout Is this a FIXME note, not sure I understand what this keans http://gerrit.cloudera.org:8080/#/c/13748/18/fe/src/test/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutorTest.java@115 PS18, Line 115: if (expected.getCause() instanceof TimeoutException) { if (!(expected.getCause() instanceof TimeoutException)) { fail("TimeoutException expected but not thrown"); } -- 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: 18 Gerrit-Owner: radford nguyen <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: radford nguyen <[email protected]> Gerrit-Comment-Date: Tue, 13 Aug 2019 18:16:20 +0000 Gerrit-HasComments: Yes
