radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13748 )
Change subject: IMPALA-8571[WIP]: improve QueryEventHook execution ...................................................................... Patch Set 18: (10 comments) Thanks for the feedback, asherman. Replies inline 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 woul Yes, this is something that should be included in the commit message 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. I was not planning on publishing the html; I have just been in the habit of using html because eclipse/Idea/etc will render it when displaying the javadoc in a tooltip window. Everything just turns into one giant line in there if you don't format. Anyway, I'll switch to markdown, since it's both human-readable and can be rendered by an IDE plugin or javadoc tool if needed. 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. re: "query-event-hook" prefix ----------------------------- So this is where the split b/w backend/frontend metrics is kind of confusing. This javadoc describes the metric names as they are collected in the frontend, and unfortunately the names do not exactly align with the backend. Part of this is due to the fact that the Java metrics are objects (e.g. Timers) that can provide multiple metrics (e.g. mean time, count). In passing the metrics to the backend, it seemed to me that the convention was to pass the raw data (e.g. mean time, count) as needed, instead of say translating the entire metric object to some similarly-capable object in the backend. The "query-event-hook" prefix only exists in the backend metric names. re: hookClass vs. method ------------------------- In this context, ${hookClass} is always the fully-qualified name of the java class that implements `QueryEventHook`, and ${hookMethod} the method name invoked on that hook. An example metric name would be: org.foo.bar.MyQueryEventHook.onQueryComplete.execution.exceptions I actually tried to make this very predictable and therefore amenable to automation/generation, but if there is some way I can improve that further, please let me know. 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 Done 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 beg Done 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 beha Done 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 :-) Done 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 Done 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 Reworded as: "Sleep much longer than `hookTimeout` to reasonably guarantee a timeout." 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)) { Done -- 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 <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: Mon, 19 Aug 2019 22:10:08 +0000 Gerrit-HasComments: Yes
