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

Reply via email to