Adar Dembo 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 15: (9 comments) OK I reviewed the tests too, though with an eye towards reducing boilerplate rather than code coverage, which I expect others have already done. http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java@91 PS14, Line 91: : : : > If I understand it correctly, you are suggesting to combine CommandLine wit Oh, I just meant that: CommandLine cmd = parser.parse(options, args); String queueSize = cmd.getOptionValue(QUEUE_SIZE_LONG_OPTION); String maxWriterThreads = cmd.getOptionValue(MAX_WRITER_THREADS_LONG_OPTION); String maxMsgBytes = cmd.getOptionValue(MAX_MESSAGE_BYTES_LONG_OPTION); conf = new SubprocessConfiguration(queueSize, maxWriterThreads, maxMsgBytes); Could become: CommandLine cmd = parser.parse(options, args); conf = new SubprocessConfiguration(cmd); And then the SubprocessConfiguration constructor would access 'cmd' to retrieve whatever configuration values it needed. But I don't have a strong opinion on this; combining SubprocessConfiguration and SubprocessCommandLineParser is fine with me. http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java: http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@51 PS15, Line 51: private static Function<Throwable, Object> errorHandler = (t) -> { Could be final? Should be named ERROR_HANDLER. http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@59 PS15, Line 59: SubprocessExecutor() { : } Can be removed? http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java@44 PS14, Line 44: @Override > In OutputStream, the other two variants will call into write(int b). But to What about close()? Do we need to provide an override for it? http://gerrit.cloudera.org:8080/#/c/14329/15/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/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@81 PS15, Line 81: @Rule : public RetryRule retryRule = new RetryRule(); Nit: move this to just after the declaration of LOG so that it's easier to find. http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@104 PS15, Line 104: } catch (TimeoutException e) { I think you can simplify this code by adding (expected = TimeoutException.class) to the @Test annotation. Then you don't need the try/catch here at all, or the fail(). Same for testMsgWithEmptyPayload. For the other tests, you'll need to use an ExpectedException rule (https://github.com/junit-team/junit4/wiki/Exception-testing#expectedexception-rule). But in order to avoid unexpected interactions with the RetryRule you'll also need to use a RuleChain. See commit ef900efdc for details. http://gerrit.cloudera.org:8080/#/c/14329/15/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/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@41 PS15, Line 41: read and write reading and writing http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@43 PS15, Line 43: public class TestMessageIO { Same comments about using an ExpectedException Rule and a RuleChain. http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@53 PS15, Line 53: @Override : public void setError() { : super.setError(); : } Should add a comment explaining what's happening here: you're expanding the visibility of setError() so that you can call it from tests. -- 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: 15 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: Wed, 05 Feb 2020 18:45:13 +0000 Gerrit-HasComments: Yes
