Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15074 )
Change subject: [java] KUDU-2972: add Kudu Ranger plugin ...................................................................... Patch Set 13: (4 comments) http://gerrit.cloudera.org:8080/#/c/15074/13/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java File java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/15074/13/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@115 PS13, Line 115: ranger nit: capitalize http://gerrit.cloudera.org:8080/#/c/15074/13/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@120 PS13, Line 120: performs nit: "who is performing" http://gerrit.cloudera.org:8080/#/c/15074/13/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java File java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java: http://gerrit.cloudera.org:8080/#/c/15074/13/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java@128 PS13, Line 128: byte[] message = MessageTestUtil.serializeMessage(subprocessRequest); : final ByteArrayInputStream in = new ByteArrayInputStream(message); : final ByteArrayOutputStream out = new ByteArrayOutputStream(); : final PrintStream outStream = new PrintStream(out, /* autoFlush= */false, "UTF-8"); : System.setIn(in); : System.setOut(outStream); : final SubprocessExecutor subprocessExecutor = new SubprocessExecutor(); : final RangerProtocolHandler protocolHandler = new RangerProtocolHandler(); : final String[] args = {""}; nit: consider reusing setUpExecutorIO() form TestEchoSubprocess? Maybe put it in some SubprocessTestUtil or somesuch. Then you can use sendRequestToPipe() and receiveResponse() below. That way you also wouldn't need to rewrite these in every new subprocess test. And you wouldn't need to rely on making unpackAndExecuteRequest(), which should be private. http://gerrit.cloudera.org:8080/#/c/15074/13/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java@137 PS13, Line 137: // Verify the ranger response. : assertThrows(TimeoutException.class, new ThrowingRunnable() { : @Override : public void run() throws Exception { : subprocessExecutor.run(args, protocolHandler, /* timeoutMs= */1000); : } : }); It isn't really clear why we're timing out here, given we successfully get responses. Mind commenting? -- To view, visit http://gerrit.cloudera.org:8080/15074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c995ac1a48ebf57667231cd3a82d3794f6ddf8d Gerrit-Change-Number: 15074 Gerrit-PatchSet: 13 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, 03 Mar 2020 22:17:13 +0000 Gerrit-HasComments: Yes
