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 12: (20 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 call it elsewhere (KuduSubprocessException, SubprocessConfiguration, etc.). It's a bit more confusing now because java.util.concurrent.Executors exists here too. 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? 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 instead say: "Wrap the system output in a SubprocessOutputStream so IOExceptions from system output are propagated up instead of being silently swallowed." 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 the entire runtime of the subprocess? Aren't they meant to be long-running? Yeah, seems like this is only used in tests to ensure our tests don't run forever. Maybe comment with something to the effect of: "In cases where we don't want our executor to run forever, e.g. in tests, wait for the specified timeout." 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? 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. "The read is blocking and not atomic (partial reads can occur if exceptions occur). As such, users should ensure this is not called from multiple threads concurrently." 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 left with some bytes written but not flushed? I guess we wouldn't ever be able to flush since subsequent calls to writeMessage() will raise exceptions after more calls to write(). 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 'this'. But according to https://www.geeksforgeeks.org/synchronized-in-java/ the effect on concurrency would be the same since these are the only synchronized blocks in this class. That said, maybe we ought to synchronize on 'out' so it's more obvious we're only worried about blocking concurrent writes? 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 se. Maybe "responseWithError" since this just creates the response? 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? Wrapper around {@link java.io.PrintStream} that throws an <code>IOException</code> instead of relying on explicit <code>checkError</code> calls when writing or flushing to the stream. This makes its error-throwing behavior more similar to most {@link java.io.OutputStream}s. 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 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? "Flushes the stream and checks its error state." from https://docs.oracle.com/javase/7/docs/api/java/io/PrintStream.html#checkError() 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? 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? 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 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 plumb some interrupt() into the Executor class itself? Then this test would look a bit more like the other tests, and we wouldn't need to reimplement this test if the guts of Executor ever change. 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, won't these just time out? 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 setError() so we can test the exception handling on the write side? 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 be more bytes than is expected? Or how about explicitly using a string that is larger than the buffer size? It can be a bit surprising to see "malformed message" be considered an exceedingly large message. 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 data in the pipe? -- 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, 27 Jan 2020 23:11:49 +0000 Gerrit-HasComments: Yes