[Impala-ASF-CR] IMPALA-8571[WIP]: improve QueryEventHook execution

["Andrew Sherman (Code Review)" <ge...@cloudera.org>]

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 <ra...@gmail.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fr...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Aug 2019 18:16:20 +0000
Gerrit-HasComments: Yes