Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15315 )
Change subject: [java] subprocess: add a metrics message ...................................................................... Patch Set 1: (8 comments) 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(); Note in a comment here what we're starting to record and who is expected to finish the recording. 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 you rename it (and update the Javadoc too)? 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 finish the recording. 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 Javadoc to make that more explicit? 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: One thing I find a little unintuitive with the current implementation is the asymmetry of the two queues: the inbound queue contains encoded request data, while the outbound queue contains decoded response data. Can we address that, either by parsing the requests when we read them off the wire, or encoding the responses after execution? On further thought, the latter won't work if there are additional metrics to calculate at the time that we write to stdout. 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 alongside outboundQueue? 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.isRunning()? 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; "...at the time that the {request,response} was queued." presumably? Having read through the Java code, this will actually reflect the queue lengths at the time that the response is sent back. Which is more useful? "At the time of queueing" seems more intuitive to me, and it's what we do in src/kudu/util/threadpool.{cc,h}. -- 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: Fri, 28 Feb 2020 19:47:07 +0000 Gerrit-HasComments: Yes
