radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13748 )
Change subject: IMPALA-8571: harden QueryEventHook execution ...................................................................... Patch Set 26: (5 comments) Addressed some nits; still need to look into simplifying the timeout monitor executor 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? Yes, I was asked to split out the metrics into a separate ticket in order to speed up acceptance of the hardening/monitoring 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 Will do 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@169 PS25, Line 169: this.metrics_ = Objects.requireNonNull(hookMetrics); : : Preconditions.checkArgument(hookTimeout >= 1, : "hook timeout should be >= 1 but was " + hookTimeout); : t > nit: Preconditions.checkArgument(hookTimeout >=1, mesg); Ooh, I also used the wrong templating format in the log message and forgot the timeout: ``` String.format("hook timeout should be >= 1 but was %s.", hookTimeout); ``` 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. Done http://gerrit.cloudera.org:8080/#/c/13748/25/fe/src/main/java/org/apache/impala/hooks/FixedCapacityQueryHookExecutor.java@190 PS25, Line 190: t- > nit: space 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: 26 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, 26 Sep 2019 23:09:55 +0000 Gerrit-HasComments: Yes
