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 5: (14 comments) http://gerrit.cloudera.org:8080/#/c/14329/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14329/5//COMMIT_MSG@22 PS5, Line 22: IOExcpetion nit: IOException 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 don't quite understand what will your proposal look like. Do you mean to Yes. Multiple jars makes more sense to me than a single JAR that has everything (Atlas, Ranger, etc.) in it, especially because it means that we can update/deprecated/etc them more independently. The idea would be that RangerProcessorMain (or similar) would define all of these overrides, as well as the members/functions in ProtocolProcessor. That way there would be a single file that defines everything we need for the Echo service, Ranger service, Atlas service, etc. And we wouldn't have to rely on passing configs to pick the service we want to run since each service would run in its own JAR. http://gerrit.cloudera.org:8080/#/c/14329/5/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/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@77 PS5, Line 77: binary mode nit: "binary mode" doesn't exist in the context of the java subprocess anymore, so maybe remove this? Same below. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@85 PS5, Line 85: * @throws IllegalArgumentException if the serialization mode is invalid : * @throws IllegalStateException if the message is malformed nit: remove this http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@88 PS5, Line 88: String readMessage() throws IOException { nit: Maybe we should add preconditions here that 'in' is set. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@88 PS5, Line 88: readMessage nit: how about calling this readBytes() or somesuch? I know we're technically reading profobuf messages, but at this point in the process, all we know is that we're dealing with bytes, and so calling it readBytes() makes it more obvious that we haven't done a whole lot of PB parsing yet. Same with parseMessage. Maybe call it parseBytes()? or bytesToMessage() or something similar? http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@117 PS5, Line 117: * @throws IllegalArgumentException if the serialization mode is invalid : * @throws InvalidProtocolBufferException if there are any unknown types : * in the message nit: remove this http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@122 PS5, Line 122: void writeMessage(Message message) throws IOException { nit: Maybe we should add preconditions here that 'out' is set. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@131 PS5, Line 131: protobuf message with the given parser and builder. nit: we're not actually parsing a protobuf message -- we're parsing some bytes and creating a protobuf message using the given parser, right? http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java: http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@33 PS5, Line 33: read nit: reads http://gerrit.cloudera.org:8080/#/c/14329/5/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/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@37 PS5, Line 37: write nit: writes http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java: http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@67 PS5, Line 67: Function<Throwable, Object> errorHandler = (t) -> { : System.exit(1); : return null; : }; nit: Could this be static? http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@78 PS5, Line 78: producer nit: we've shifted over to calling these 'readers' and 'writers'. Could you update variable names, comments, and tests accordingly? http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/MessageTest.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/MessageTest.java: http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/MessageTest.java@69 PS5, Line 69: MessageProcessor messageProcessor = new MessageProcessor( : SubprocessConfiguration.MAX_MESSAGE_BYTES_DEFAULT); Do we have to have a different constructor for this? Couldn't we just pass in byteOutputStream? Same below? -- 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: 5 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: Mon, 09 Dec 2019 22:59:58 +0000 Gerrit-HasComments: Yes
