radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12914 )
Change subject: IMPALA-8226: Add grant/revoke to/from group for Ranger ...................................................................... Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/12914/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12914/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@193 PS7, Line 193: String user, List<String> groups, String clusterName, List<TPrivilege> privileges) { nit: We could probably use a `Collection<String> groups` to be more general here, since the group's items are copied into a `List` when creating the request. Same with `privileges`. http://gerrit.cloudera.org:8080/#/c/12914/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@233 PS7, Line 233: if (!groups.isEmpty()) request.getGroups().addAll(groups); nit: is the `if` statement really necessary given the contract of `addAll`? http://gerrit.cloudera.org:8080/#/c/12914/10/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12914/10/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@170 PS10, Line 170: "%s is not supported in Impalad", ClassUtil.getMethodName())); Isn't it more accurate to say that this isn't supported with sentry? -- To view, visit http://gerrit.cloudera.org:8080/12914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28b7b3e4c776ad1bb5bdc184c7d733d0b5ef5e96 Gerrit-Change-Number: 12914 Gerrit-PatchSet: 11 Gerrit-Owner: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: radford nguyen <radford.ngu...@gmail.com> Gerrit-Comment-Date: Thu, 04 Apr 2019 19:57:09 +0000 Gerrit-HasComments: Yes
