Andrew Wong 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 10: (13 comments) http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@39 PS10, Line 39: MessageProcessor nit: there isn't a whole lot of processing being done in this class. It seems squarely focused on reading from the pipe and writing to the pipe. Maybe rename this MessageIO or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@130 PS10, Line 130: <T extends Message> T bytesToMessage(byte[] data, Parser<T> parser) Can this be static? Why is this in this class? http://gerrit.cloudera.org:8080/#/c/14329/10/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/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@50 PS10, Line 50: blockingQueue nit: since we're expecting there to eventually be an outbound queue, maybe call this inboundQueue or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@46 PS10, Line 46: blockingQueue nit: same here, maybe rename this to inboundQueue? Especially if the MessageWriter will interact with the outboundQueue. http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@48 PS10, Line 48: private ProtocolProcessor protocolProcessor; nit: not really sure what protocol is referring to here. Maybe call this RequestHandler requestHandler? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@107 PS10, Line 107: messageProcessor.bytesToMessage( : data, SubprocessRequestPB.parser()); This doesn't use any of the MessageProcessor's internal state, so maybe just inline the call here? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@44 PS10, Line 44: Subprocess.SubprocessResponsePB processRequest(Subprocess.SubprocessRequestPB request) nit: maybe call this handleSubprocessRequest() or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@58 PS10, Line 58: Responds nit: this doesn't actually send a response; maybe rename it and update this to focus on the fact that it's doing stuff with the request? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@63 PS10, Line 63: responses nit: maybe call this handleRequest() or something? http://gerrit.cloudera.org:8080/#/c/14329/10/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/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@40 PS10, Line 40: THREAD nit: this should be plural http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java@60 PS10, Line 60: byte[] malformedMessage = "malformed message".getBytes(StandardCharsets.UTF_8); This doesn't look like it's creating a message that is longer than 1024*1024 bytes. Mind commenting why this is malformed? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageReaderWriter.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageReaderWriter.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageReaderWriter.java@119 PS10, Line 119: // parsing non-malformed message exits normally without any exceptions. : {"test", false, false, false, false, false, noErrors}, : // parsing empty input message exits normally without any exceptions. : {"", false, true, false, false, false, noErrors}, : // parsing message with empty payload exits normally without any exceptions. : {emptyPayload, true, true, false, false, false, noErrors}, : // parsing malformed message causes IOException. : {"malformed", false, true, true, false, false, hasError}, : // parsing message with IOException injected exits with exception. : {"test", false, false, false, true, false, hasError}, : // parsing message with InterruptedException injected exits with exception. : {"test", false, false, false, false, true, hasError}, nit: IMO this makes this test significantly more difficult to read. Maybe consider instead having a test method that takes as a MessageProcessor, MessageReader, MessageWriter, and a message, and returns the output. And then define multiple test cases that each use this method but set up the inputs with different failure modes, and then inspect the output/exceptions/etc in each test case. I think that'd make it much easier to correspond bad behavior with the expected error. http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageReaderWriter.java@142 PS10, Line 142: if (!isSerialized) { : request = MessageTestUtil.createEchoSubprocessRequest(message); : messageBytes = MessageTestUtil.serializeMessage(request); : } else { : messageBytes = message.getBytes(StandardCharsets.UTF_8); : } I might be missing something here. Mind adding a comment what we're testing here and why? Like why are we serializing the message isSerialized is false? -- 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: 10 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[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, 16 Jan 2020 23:41:30 +0000 Gerrit-HasComments: Yes
