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 14: (5 comments) 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: Assert.assertTrue > Sure, I agree that it doesn't feel great to intermingle test-only code with Hmm, it is actually hard to not include/use CompletableFuture(no Executor) in this case as we want to verify the error handling of Executor as well. So after pondering a bit, I took a step back to plumb the inject state into Executor. I try to make the code looks clean, but let me know how you think. Thanks! http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@210 PS12, Line 210: : > I don't really follow this comment. Mind rewriting it? Also probably don't Removed. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@214 PS12, Line 214: : > Could we move the readerFuture.get() call out of the try/catch? Removed. 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: > Having explicit tests for each component of a large system in isolation see Discussed offline, now I got Andrew's comment is about test SubprocessOutputStream. Updated to add such case. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@75 PS12, Line 75: * catch errors thrown from underlying <code>PrintStream</code> and r > Say we send over the following message: Oh, this test case is for such scenario (mismatched size and body). I will update it to be more specific. -- 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: 14 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: Tue, 04 Feb 2020 07:47:14 +0000 Gerrit-HasComments: Yes
