Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11314 )
Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER ...................................................................... Patch Set 20: (9 comments) http://gerrit.cloudera.org:8080/#/c/11314/22/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/11314/22/common/thrift/JniCatalog.thrift@77 PS22, Line 77: // The server name for security privileges when authorization enabled. > nit: is Done http://gerrit.cloudera.org:8080/#/c/11314/22/common/thrift/JniCatalog.thrift@131 PS22, Line 131: // The server name for security privileges when authorization enabled. > nit: is (several more below) Done http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2660 PS22, Line 2660: Checks for authorization being enabled and returning the appropriate value for : * server name. > simplify: Returns the server name if authorization is enabled. Returns null Done. Yes. http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1005 PS22, Line 1005: > nit: use /** comments. Done http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/service/Frontend.java@488 PS22, Line 488: == TPrincipalType.USER) { > pull up to prev. line Makes the line length 92. http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java File fe/src/main/java/org/apache/impala/util/SentryPolicyService.java: http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java@485 PS22, Line 485: entry > Sentry? .. what's 'entry'? Done http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/test/resources/mysql-hive-site.xml.template File fe/src/test/resources/mysql-hive-site.xml.template: http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/test/resources/mysql-hive-site.xml.template@a116 PS22, Line 116: > I was going to comment on why I don't see this file removed but I don't see It's been unused as far as I know in development. It appears in the documentation so may be used in production. http://gerrit.cloudera.org:8080/#/c/11314/22/fe/src/test/resources/mysql-hive-site.xml.template@115 PS22, Line 115: hive.sentry.conf.url > why this name change in this patch? Because hive.access.conf.url doesn't work. impalad.INFO:W0906 10:27:26.005672 25891 HiveConf.java:4043] HiveConf of name hive.access.conf.url does not exist http://gerrit.cloudera.org:8080/#/c/11314/20/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11314/20/tests/authorization/test_owner_privileges.py@250 PS20, Line 250: __root_query_fail > The change is quite different than what I had in mind. Done -- To view, visit http://gerrit.cloudera.org:8080/11314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0 Gerrit-Change-Number: 11314 Gerrit-PatchSet: 20 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 06 Sep 2018 15:33:57 +0000 Gerrit-HasComments: Yes
