Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/14106 )
Change subject: IMPALA-8228: Ownership support for Ranger authz ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java File fe/src/main/java/org/apache/impala/authorization/Authorizable.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java@62 PS6, Line 62: return getName().hashCode(); If we're comparing owner, we should update the hashCode() as well. http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java@70 PS6, Line 70: temp.getOwnerUser() == this.getOwnerUser() Shouldn't this use equals()? http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@31 PS6, Line 31: private final String ownerUser_; For consistency, annotate with @Nullable. http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@34 PS6, Line 34: String ownerUser I think it's more readable to also annotate the parameter with @Nullable. http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java@33 PS6, Line 33: String ownerUser Same comment about @Nullable parameter. http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java@45 PS6, Line 45: /** : * Creates a new instance of column {@link Authorizable} for a given database name and : * gives access to all tables and columns. : */ : Authorizable newColumnAllTbls(String dbName, @Nullable String dbOwnerUser); : : /** : * Creates a new instance of column {@link Authorizable} for given database and table : * names and gives access to all columns. : */ : Authorizable newColumnInTable( : String dbName, String tableName, @Nullable String tblOwnerUser); : : /** : * Creates a new instance of column {@link Authorizable} for given database, table, and : * column names. : */ : Authorizable newColumnInTable( : String dbName, String tableName, String columnName, @Nullable String tblOwnerUser); nit: update javadoc about the new owner parameter. http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java@34 PS6, Line 34: String ownerUser Same comment about @Nullable parameter. http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@94 PS6, Line 94: resources.add(new RangerImpalaResourceBuilder() : .database("*").table("*").column("*").build()); : resources.add(new RangerImpalaResourceBuilder() : .database("*").function("*").build()); : resources.add(new RangerImpalaResourceBuilder().uri("*").build()); Should we add .owner() here as well? http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/catalog/Db.java@544 PS6, Line 544: if (db == null) return null; : return db.getOwnerName(); nit: can be inlined: return db == null ? null : db.getOwnerName() -- To view, visit http://gerrit.cloudera.org:8080/14106 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I737b7164a3e7afb9996b3402e6872effd663f7b4 Gerrit-Change-Number: 14106 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fre...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 11 Sep 2019 03:59:15 +0000 Gerrit-HasComments: Yes
