Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] KUDU-2971: process communicates via protobuf-based protocol ...................................................................... Patch Set 18: (8 comments) http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java: http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@83 PS17, Line 83: read the specifie > nit: "If it fails to read" Done http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@123 PS17, Line 123: ta a four-byt > nit: "a four-byte array", same below Done http://gerrit.cloudera.org:8080/#/c/14329/17/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/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@88 PS17, Line 88: it up the call sta > nit: "propagate it up the call stack" Done http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java: http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@33 PS17, Line 33: private int queueSize; > What's the point of defining all these variables up front instead of in-lin Done http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@34 PS17, Line 34: > nit: camelCase for this? Done http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@36 PS17, Line 36: AULT = 3; > nit: maybe "Maximum number of messages held by the message queue"? so it's Done http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java: http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@95 PS17, Line 95: */ : @Test(expected = TimeoutException.class) : public void testBasicMsg() throws Exception { : final String message = "data"; : fi > nit: maybe declare this to be static as NO_ERRORS and reuse it elsewhere. S Done http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@180 PS17, Line 180: > nit: why not inline this and annotate with a comment? Same above? Done -- To view, visit http://gerrit.cloudera.org:8080/14329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf9ad24dbc9acc681284b6433836271b5b4c7982 Gerrit-Change-Number: 14329 Gerrit-PatchSet: 18 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Feb 2020 21:33:24 +0000 Gerrit-HasComments: Yes
