Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17640 )
Change subject: IMPALA-10436: Support storage handler privileges for external Kudu table creation ...................................................................... Patch Set 9: (5 comments) Sorry to be late here.. I just have some minor comments which are ok to be ignored if you need to merge this patch soon. http://gerrit.cloudera.org:8080/#/c/17640/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17640/9//COMMIT_MSG@14 PS9, Line 14: supported by Apache Ranger once RANGER-3281 is resolved, which in turn : depends on the release of Apache Hive 4.0 that consists of HIVE-24705. I see HIVE-24705 is resolved but RANGER-3281 is still open. Do we need to bump the GBN before merging this patch? http://gerrit.cloudera.org:8080/#/c/17640/9/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/17640/9/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@61 PS9, Line 61: import com.google.common.collect.Lists; nit: move this to line 59 to keep the import list sorted http://gerrit.cloudera.org:8080/#/c/17640/9/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@369 PS9, Line 369: // Today there is no comprehensive way of enforcing a Ranger authorization policy : // against tables stored in Kudu. This is why only users with ALL privileges on : // SERVER may create external Kudu tables or set the master addresses. : // See IMPALA-4000 for details. Should we update these? http://gerrit.cloudera.org:8080/#/c/17640/9/fe/src/main/java/org/apache/impala/analysis/StorageHandlerUri.java File fe/src/main/java/org/apache/impala/analysis/StorageHandlerUri.java: http://gerrit.cloudera.org:8080/#/c/17640/9/fe/src/main/java/org/apache/impala/analysis/StorageHandlerUri.java@37 PS9, Line 37: storageHandlerUri.compareTo("*://*") == 0 nit: we can use equals() directly storageHandlerUri.equals("*://*") http://gerrit.cloudera.org:8080/#/c/17640/9/fe/src/main/java/org/apache/impala/analysis/StorageHandlerUri.java@47 PS9, Line 47: compareTo("*") == 0 nit: use equals() directly -- To view, visit http://gerrit.cloudera.org:8080/17640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7936e1d8c48696169f7ad7ad92abe44a26eea3c4 Gerrit-Change-Number: 17640 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Mon, 31 Oct 2022 02:33:30 +0000 Gerrit-HasComments: Yes
