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 13: (6 comments) http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/Executor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/Executor.java: http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/Executor.java@48 PS12, Line 48: > Maybe name it SubprocessExecutor? Subprocess is not available as the protob Sure http://gerrit.cloudera.org:8080/#/c/14329/12/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/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@195 PS12, Line 195: MessageReader reade > Yeah, I understand, but I chose not to do that as it would be weird to plum Sure, I agree that it doesn't feel great to intermingle test-only code with non-test code. My overall feedback on this test is that it's pretty cluttered, and that makes it difficult to reason about what it's actually testing. FWIW, if the goal is to test the specific behavior of MessageWriter, it might make sense to have tests that only instantiate a MessageWriter and a blocking queue (ie no Executor), and then manually inserting to the blocking queue and testing the behavior of the MessageWriter. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@210 PS12, Line 210: CompletableFuture writerFuture = : CompletableFuture.runAsync(writer, writerService); I don't really follow this comment. Mind rewriting it? Also probably don't need all the parens? http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@214 PS12, Line 214: : > The writer task never timeout and will block on getting new messages. Could we move the readerFuture.get() call out of the try/catch? http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java: http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@39 PS12, Line 39: public class TestMessageIO { > So what is the purpose of such test in MessageIO? I thought we want that at Having explicit tests for each component of a large system in isolation seems like a good idea. That way if the exception-handling of MessageIO ever changes for whatever reason, we don't have to debug more complex tests that involve entire Subprocesses -- we would see TestMessageIO fail and would clued into the fact that MessageIO is probably to blame. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@75 PS12, Line 75: size = MessageIO.intToBytes(100); > Can you be more specific about what "isn't enough data" mean in this case? Say we send over the following message: [1000][10 bytes of stuff] We should see an EOFException or something, right? -- 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: 13 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 03 Feb 2020 08:14:08 +0000 Gerrit-HasComments: Yes
