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: (5 comments) http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java File java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java: http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java@122 PS1, Line 122: Sends > nit: Send. Not sure I agree here, given we describe methods with adjectives instead of verbs elsewhere. Or is there something about this in a style guide that I missed? http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java@128 PS1, Line 128: Receives > nit: Receive Not sure I agree here, given we describe methods with adjectives instead of verbs elsewhere. Or is there something about this in a style guide that I missed? http://gerrit.cloudera.org:8080/#/c/15315/1/java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java@135 PS1, Line 135: Sets > nit: Set Not sure I agree here, given we describe methods with adjectives instead of verbs elsewhere. Or is there something about this in a style guide that I missed? 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 > Is this required SubprocessMetrics has started timing before the initializa No, it's only required that the timer is started before adding it to the queue. 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@71 PS1, Line 71: reset > Why use reset not stop here? I'll update the comment, but reset() lets us begin timing a new section the next time we start the timer. If we call stop(), the timer will carry on from where it left off (at least I assume so, based on the docs), which isn't what we want if we're timing non-overlapping periods of time. https://guava.dev/releases/19.0/api/docs/com/google/common/base/Stopwatch.html -- 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 07:58:48 +0000 Gerrit-HasComments: Yes
