Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@59 PS9, Line 59: PartitionSpec partitionSpec, String db, boolean authorization) { We now have two flags: isRefresh, and authorization. Are these disjoint? One is true or the other? And, how to they relate to partitionSpec? Should we define an enum to identify the meaning of the statement rather than parsing it from flags? enum Action{ REFRESH_AUTH, REFRESH_TABLE, REFRESH_ALL, ... }; http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@111 PS9, Line 111: if (authorization_) { The logic here is getting hard to follow. Would be easier using a case statement with that enum mentioned above. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@160 PS9, Line 160: result.append(" AUTHORIZATION"); Same comment as above. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1140 PS9, Line 1140: if (sentryProxy_ != null) { if (sentryProxy == null) return; Then, outdent the try/catch block. Result is a bit easier to read. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/jflex/sql-scanner.flex@78 PS9, Line 78: keywordMap.put("authorization", SqlParserSymbols.KW_AUTHORIZATION); A persistent concern when adding keywords is that some queries will now fail: SELECT authorization FROM mySecurityTable Is there some existing keyword that could be used instead, even if not ideal? Maybe REFRESH ROLE METADATA? REFRESH GRANT METADATA? http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@331 PS9, Line 331: AnalyzesOk("refresh authorization", createAnalysisCtx(createAuthorizationConfig())); Ideally, here or elsewhere, we'd inspect the resulting statement to ensure all the knick-knacks are set as you expect: flags, fields, etc. Ensure that predicates fire as you expect: is/is not a table action is an authorization action, etc. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@451 PS9, Line 451: AuthorizationConfig authzConfig = AuthorizationConfig.createHadoopGroupAuthConfig( Maybe explain this a bit: what is covered in the file? What does this enabled? When should we use this form? http://gerrit.cloudera.org:8080/#/c/11888/9/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/11888/9/tests/authorization/test_authorization.py@547 PS9, Line 547: self.client.execute("refresh authorization") Not specific to this test, but what do we return from this statement? Should we return a warning or message if no auth is configured? A success message or code if successful? -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Wed, 05 Dec 2018 21:42:38 +0000 Gerrit-HasComments: Yes
