Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )
Change subject: IMPALA-8309: add user authorization_provider flag ...................................................................... Patch Set 10: (16 comments) http://gerrit.cloudera.org:8080/#/c/12901/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12901/10//COMMIT_MSG@23 PS10, Line 23: : Manually started minicluster with each of following flags : and verified correct authorization strategy chosen: : : - provider='' factory='' => sentry : - provider=sentry factory='' => sentry : - provider=ranger factory='' => ranger : - provider='' factory=sentry => sentry : - provider='' factory=ranger => ranger : - provider=sentry factory=sentry => sentry : - provider=ranger factory=sentry => sentry : - provider=sentry factory=ranger => ranger : - provider=ranger factory=ranger => ranger : : Wrote unit tests to capture above assertions : : Ran fe unit and e2e tests : : Wrote e2e test to verify new flag behavior nit: add bullet points for each item? http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java: http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@30 PS10, Line 30: This is : * typically used only for logging. this comment is not necessary http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java: http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@35 PS10, Line 35: factoryClassName nit: the naming convention is factoryClassName_ http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@37 PS10, Line 37: AuthorizationProvider(String factoryClassName) { : this.factoryClassName = factoryClassName; : } : : /** : * Returns the canonical name of the {@link AuthorizationFactory} implementation : * associated with this provider. : * : * @return the canonical name of the {@link AuthorizationFactory} impl for `this` : */ : public String getAuthorizationFactoryClassName() { : return this.factoryClassName; : } nit: remove this. http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/BackendConfig.java@31 PS10, Line 31: nit: remove extra new line http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java@116 PS10, Line 116: final AuthorizationFactory authzFactory : = JniFrontend.authzFactoryFrom(BackendConfig.INSTANCE); sharing code between JniCatalog and JniFrontend is weird. Maybe create a helper class or helper function elsewhere? http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java@791 PS10, Line 791: ); nit: move to L790 http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java@796 PS10, Line 796: ); nit: move to L795 http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java@824 PS10, Line 824: ); nit: move to L823 http://gerrit.cloudera.org:8080/#/c/12901/10/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/12901/10/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@326 PS10, Line 326: public String getProviderName() { : return "noop"; : } nit: can be put in a single line http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java: http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@81 PS10, Line 81: private static BackendConfig authCfg( the whole thing here should be in a separate test class http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@102 PS10, Line 102: _onlyProviderSpecified nit: don't use underscore http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@115 PS10, Line 115: _FactoryTakesPrecedenceOverProvider nit: don't use underscore http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py File tests/authorization/test_provider.py: http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py@28 PS10, Line 28: class TestProviderFails(CustomClusterTestSuite): I think we can just put the test in test_authorization.py instead. http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py@43 PS10, Line 43: nit: remove new line http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py@48 PS10, Line 48: nit: remove new line -- To view, visit http://gerrit.cloudera.org:8080/12901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19 Gerrit-Change-Number: 12901 Gerrit-PatchSet: 10 Gerrit-Owner: radford nguyen <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: radford nguyen <[email protected]> Gerrit-Comment-Date: Wed, 24 Apr 2019 16:55:40 +0000 Gerrit-HasComments: Yes
