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 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/15074/3/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/3/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@30 PS3, Line 30: public class RangerKuduAuthorizer { Should we also use mockito to test this class by generating some expected RangerAccessResults and explicitly denoting whether they result in a successful authorization? http://gerrit.cloudera.org:8080/#/c/15074/3/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@66 PS3, Line 66: RangerAccessResult If authorize() is to return false, maybe we should also debug log the RangerAccessResult.toString() http://gerrit.cloudera.org:8080/#/c/15074/3/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@67 PS3, Line 67: result.getIsAccessDetermined(); Probably meant to do something with this? Do we need to retry if this is false? http://gerrit.cloudera.org:8080/#/c/15074/3/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/3/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java@26 PS3, Line 26: import org.apache.kudu.subprocess.SubprocessExecutor; nit: reorder? http://gerrit.cloudera.org:8080/#/c/15074/3/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java@69 PS3, Line 69: public void testBasicRangerMessage() throws Exception { Maybe consider a test that sends multiple requests before serializing? And also mocking a not-allowed case? -- 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: 3 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 04 Feb 2020 01:06:30 +0000 Gerrit-HasComments: Yes
