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 12: (21 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: Executor > nit: FWIW I think Subprocess for this was fine, considering that's what we Maybe name it SubprocessExecutor? Subprocess is not available as the protobuf file has the name. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/Executor.java@50 PS12, Line 50: private Function<Throwable, Object> errorHandler; > Can this be static final too? Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/Executor.java@88 PS12, Line 88: // need to 1. ensure no partial message write due to auto flush : // 2. PrintStream is sawlloing IOexception > nit: so it's clear you're referring to the SubprocessOutputStream, maybe in Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/Executor.java@112 PS12, Line 112: else { : readerFuture.get(timeoutMs, TimeUnit.MILLISECONDS); : CompletableFuture.allOf(writerFutures) : .get(timeoutMs, TimeUnit.MILLISECONDS); : } > I'll probably get to this later, but why would we impose a time limit on th Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/KuduSubprocessException.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/KuduSubprocessException.java: http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/KuduSubprocessException.java@29 PS12, Line 29: subprocess > Remove? Done http://gerrit.cloudera.org:8080/#/c/14329/12/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/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@53 PS12, Line 53: The read is not atomic (partial read can happen if any : * exceptions occur) and blocking (waits until the input is available). > nit: maybe be more prescriptive here? E.g. Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@103 PS12, Line 103: The write is atomic, that is if any exceptions occur, no partial : * message should be written to the underlying output stream. > Is it possible that we'll raise an exception in flush(), and then we're lef By 'exception' you mean IOException, right? Yeah, I think so but that will cause the program to exit as well, so it wouldn't matter for the other end to receive partial message. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@113 PS12, Line 113: this > nit: at first I thought we might want to synchronize on 'out' instead of 't Done http://gerrit.cloudera.org:8080/#/c/14329/12/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/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@91 PS12, Line 91: respondWithError > nit: this doesn't actually send the response so it's not "responding" per s Done http://gerrit.cloudera.org:8080/#/c/14329/4/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/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@95 PS4, Line 95: > Done Sorry, revisiting the comment here.. Actually in C++ side we are using int for max message bytes, https://github.com/apache/kudu/blob/master/src/kudu/subprocess/subprocess_protocol.cc#L45. Did I miss anything? http://gerrit.cloudera.org:8080/#/c/14329/12/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/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java@25 PS12, Line 25: * The {@link SubprocessOutputStream} class is a wrapper around {@link PrintStream} : * for explicitly re-throw <code>IOException</code> if any encountered in a : * <code>PrintStream</code>. Because unlike other output streams, a <code>PrintStream</code> : * never throws an <code>IOException</code>; instead, exceptional situations : * merely set an internal flag that can be tested via the <code>checkError</code> : * method. > nit: looking around for other wrappers, maybe reword a bit? Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java@47 PS12, Line 47: @Override > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java@49 PS12, Line 49: out.flush(); > Seem like we might not need this? Done 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@90 PS12, Line 90: Assert.assertTrue(false); > Could we also display the error here? You mean log the error if it ever happen? If so, done. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@156 PS12, Line 156: String[] args = {"-w", "1"}; > Is this important for this test? Yeah, added the comment to explain why. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@194 PS12, Line 194: if needed. > remove this Done http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@195 PS12, Line 195: reader.interrupt(); > Rather than copying the guts of Executor in this test, could we instead plu Yeah, I understand, but I chose not to do that as it would be weird to plumb the injection state to the Executor, it is not relevant to non-test cases at all and it is particular for MessageWriter. Since the focus is mainly on MessageWriter, I don't expect it will change much with the change of Executor. Let me know if you think otherwise or you think there is a good way to plumb the injection state. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@214 PS12, Line 214: CompletableFuture.anyOf(writerFutures) : .get(1000, TimeUnit.MILLISECONDS); > Can't we wait for all of them to finish? If there's nothing in the pipe, wo The writer task never timeout and will block on getting new messages. 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 { > How difficult would it be to plumb a PrintStream down and then call setErro So what is the purpose of such test in MessageIO? I thought we want that at TestEchoSubprocess to ensure IOException will cause the program to exit. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@58 PS12, Line 58: // Construct a message that it is not properly serialized with the message size : // that causes maximum message size exceed exception. : byte[] malformedMessage = "malformed message".getBytes(StandardCharsets.UTF_8); > Can you be more explicit and explain the UTF-8 will expand this message to Hmm, to avoid confusion, I update it to be with the size exceeds max explicitly. http://gerrit.cloudera.org:8080/#/c/14329/12/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@75 PS12, Line 75: byte[] body = "malformed message".getBytes(StandardCharsets.UTF_8); > Does it make sense to add a test for what happens if there isn't enough dat Can you be more specific about what "isn't enough data" mean in this case? -- 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: 12 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 06:55:55 +0000 Gerrit-HasComments: Yes
