Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15074 )
Change subject: [java] KUDU-2972: add Kudu Ranger plugin ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/build.gradle File java/kudu-ranger/build.gradle: http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/build.gradle@21 PS4, Line 21: // Note: We don't use the shaded version to avoid protobuf message type : // got shaded and causes incompatible bounds error for type casting. > Sorry for the confusion, if we are used shaded version, we can get compile OK, so given that it produces a compilation error, we don't need to go into great detail (it's very easy for anyone to reproduce in the future). Something like "Note: using the shaded version will result in a compilation error, so we must use the unshaded version". http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java File java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java: http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java@42 PS4, Line 42: > This cannot be final as it is be mocked in the test. Set it to static to be Please doc the mocking requirement. http://gerrit.cloudera.org:8080/#/c/15074/4/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/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@45 PS4, Line 45: private static final String APP_ID = "kudu"; > Cannot make it final as it is used in the mock test. Can you document that? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@52 PS4, Line 52: RangerBasePlugin plugin; : : public RangerKuduAuthorizer() { : > Make it a separate init() as we are mocking RangerKuduAuthorizer in the tes Could you document that requirement in the init() Javadoc? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@61 PS4, Line 61: plugin.init(); : } : : /** : * Authorizes a given <code>RangerRequestListPB</code> in Ran > Done FYI, Guava's @Nullable annotations are "self-documenting" and can also be used by static analysis tools. http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@126 PS4, Line 126: final RangerAccessRequestImpl request = new RangerAccessRequestImpl(); : request.setResource(resource); : request.setAccessType(action); > It is using hadoop user group mapping information (https://hadoop.apache.or Do we know which group mapping mechanism will be used here? Sounds like it might be org.apache.hadoop.security.JniBasedUnixGroupsMappingWithFallback, since that's the default and I don't think we go out of our way to configure a different one. Does that mean that the node running kudu-ranger must have all user/group information available to it at the OS level, in /etc/passwd and /etc/group? Is that a reasonable requirement for Kudu masters? -- 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: 6 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: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 18 Feb 2020 21:13:32 +0000 Gerrit-HasComments: Yes
