Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15315 )
Change subject: [java] subprocess: add a metrics message ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/InboundRequest.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/InboundRequest.java: http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/InboundRequest.java@21 PS1, Line 21: It is expected that the : * <code>SubprocessMetrics</code> have begun timing the time it takes to make : * it through the queue > No, it's only required that the timer is started before adding it to the qu To make it more obvious, I've commented that it's required, even if it's not a strict requirement. http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageParser.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageParser.java: http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageParser.java@69 PS1, Line 69: metrics.startTimer(); > +1, as of now it is a bit confusing that we call startTimer twice here. Done http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageParser.java@97 PS1, Line 97: private SubprocessResponsePB.Builder getResponseBuilder(byte[] data) { > It's not obvious from the name that this also executes the request. Could y Done http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java: http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@83 PS1, Line 83: metrics.startTimer(); > Note in a comment here what we're starting to record and who is expected to Done http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java: http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java@64 PS1, Line 64: protected abstract ResponseT createResponse(RequestT request); > This too involves executing the request; could you rename and update the Ja Done http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java: PS1: > Yeah, I think the guiding principle here was "do as little as we can in the I'm going to punt on this unless you feel strongly about it. I'm pretty sure that in parsing, we're doing a decent amount of work, so I'd rather keep it in the parser threads. And you're right about the metrics calculation -- I don't see a great way to write the metrics after already serializing the response to a byte array. http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@95 PS1, Line 95: BlockingQueue<InboundRequest> inboundQueue = : new ArrayBlockingQueue<>(queueSize, /* fair= */true); > For symmetry, how do you feel about declaring this as a class field alongsi Done http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMetrics.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMetrics.java: http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMetrics.java@57 PS1, Line 57: public void recordInboundQueueTimeMs() { > In the various record functions could we precondition on stopwatch.isRunnin Done http://gerrit.cloudera.org:8080/#/c/15315/1/src/kudu/subprocess/subprocess.proto File src/kudu/subprocess/subprocess.proto: http://gerrit.cloudera.org:8080/#/c/15315/1/src/kudu/subprocess/subprocess.proto@49 PS1, Line 49: // Number of requests in the subprocess's inbound queue. : optional int64 inbound_queue_length = 3; : : // Number of responses in the subprocess's outbound queue. : optional int64 outbound_queue_length = 4; > Oh, I think I see what you mean. And yeah, the threadpool metric is a histo Done -- To view, visit http://gerrit.cloudera.org:8080/15315 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11a89fff8df23c5057c577f2aebfd40922d01e3c Gerrit-Change-Number: 15315 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 02 Mar 2020 23:28:23 +0000 Gerrit-HasComments: Yes
