Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] KUDU-2791: process communicates via protobuf-based protocol ...................................................................... Patch Set 4: (19 comments) Didn't do a full review of the tests. Overall looking much better! http://gerrit.cloudera.org:8080/#/c/14329/4//COMMIT_MSG Commit Message: PS4: Could you describe the expectations w.r.t error handling? What kind of "errors" are OK? What errors return a response? What errors are fatal? What happens when there is a fatal error? How do interrupts tie into this mental model for error handling? http://gerrit.cloudera.org:8080/#/c/14329/4//COMMIT_MSG@22 PS4, Line 22: simply extend the 'ProtocolProcessor' Seems you also have do update the SubprocessConfiguration http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java@29 PS4, Line 29: @InterfaceAudience.Private I like the use of generics here. Could we extend it even further so that we only have to define a single EchoProcessorMain.java or RangerProcessorMain.java or AtlasProcessorMain.java when we add new processes, rather than adding an EchoProtocolProcessor.java _and_ updating SubprocessConfiguration.java? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java@32 PS4, Line 32: private static String ECHO_MESSAGE = "echo"; nit: final? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java@35 PS4, Line 35: /** : * Message consumer class that retrieve a message from the queue at a time. : * Then process the message and write the response to the underlying output : * stream. : */ nit: maybe note the concurrency expectations of this method, e.g. that we can expect to be running multiple MessageConsumers with a single, shared BufferedOutputStream http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java@47 PS4, Line 47: private AtomicInteger retries; Could this be local to each call to run()? If so, could it be non-atomic? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java@115 PS4, Line 115: // Restore the interrupted status to the code higher up : // on the call stack. : if (interrupted) { : Thread.currentThread().interrupt(); : } This never gets reset to false, even if we were eventually table to take from the queue. What purposes does throwing an exception serve? Don't we only get here if we're throwing an exception anyway? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@39 PS4, Line 39: class MessageProcessor { Why does this need to be a class? Couldn't it be a set of utility functions that reference Subprocess.MAX_MESSAGE_BYTES_DEFAULT? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@40 PS4, Line 40: : public enum SerializationMode { : /** : * Each message is serialized as a four byte big-endian size followed by : * the protobuf-encoded message itself. : */ : PB : } Remove this? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@50 PS4, Line 50: private static final int OFFSET = 0; nit: probably don't need to define a constant for this http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@75 PS4, Line 75: return data; nit: don't need the local variable -- just: return new String() ? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@93 PS4, Line 93: void writeMessage(BufferedOutputStream out, : Message message) throws IOException { : writeBinaryMessage(out, message); : } Why have this when writeBinaryMessage already exists? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@113 PS4, Line 113: * Read a protobuf message in {@link SerializationMode#PB} mode from : * the given buffered input stream. nit: maybe note that this isn't threadsafe and that we expect there to be a single reader of 'in' http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@126 PS4, Line 126: Preconditions.checkState(read == Integer.BYTES, : "unable to receive message size"); : int size = bytesToInt(sizeBytes); : Preconditions.checkState(size <= maxMessageBytes, : "exceeds maximum message size"); Can we also include 'read' and 'size' in the error messages to facilitate debugging? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java@39 PS4, Line 39: should not be called concurrently unless handled by : * the caller. I'm finding this a bit confusing: under what scenario can run() be called concurrently? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java@98 PS4, Line 98: continue; Why aren't we responding with an error here? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@37 PS4, Line 37: SubprocessRequestPB SubprocessResponsePB Same below http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@64 PS4, Line 64: abstract Class<Request> getRequestClazz(); Will this always be: Class<Request> getRequestClazz() { return Request.class; } ? If so, could we implement it here instead of in the subclasses? Alternatively, could we inline the call so it's something like: getResponse(request.getRequest.unpack(Request.class)) ? http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java@42 PS4, Line 42: * Parameterized the message processor test to run in both serialization : * mode and message request type. There's only one serialization mode now. -- 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: 4 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[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: Tue, 19 Nov 2019 02:36:50 +0000 Gerrit-HasComments: Yes
