Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )
Change subject: IMPALA-8309: add user authorization_provider flag ...................................................................... Patch Set 2: (13 comments) You also need to add E2E tests in the python testing. http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@41 PS2, Line 41: "sentry", nit: move default to line above like other DEFINE http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@45 PS2, Line 45: "", nit: move default to line above like other DEFINE. http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@46 PS2, Line 46: "Specifies the class name that implements the authorization provider. " : "This will override the authorization_provider flag if both are specified." nit: Wrap at the character limit not at the end of a sentence. http://gerrit.cloudera.org:8080/#/c/12901/2/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/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@31 PS2, Line 31: factoryClass Should this be `factoryClassName`? http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@67 PS2, Line 67: public String getProviderName() { return "none"; } Can we make "none" a public static final String? http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java: http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java@45 PS2, Line 45: public String getProviderName() { return "ranger"; } Can we make "ranger" a public static final String? http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java: http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java@130 PS2, Line 130: public String getProviderName() { return "sentry"; } Can we make "sentry" a public static final String? http://gerrit.cloudera.org:8080/#/c/12901/2/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/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141 PS2, Line 141: public @Nullable String getAuthorizationFactoryClassOrNull() { > Not sure if there's a standard for handling optional flags... at least I di nit: I'm not opposed to adding the @Nullable annotation and adding `orNull` in the class name but it is inconsistent with the rest of the code base. http://gerrit.cloudera.org:8080/#/c/12901/2/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/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@773 PS2, Line 773: * @param beCfg : * @return : * @throws InternalException Finish the documentation http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@778 PS2, Line 778: throws InternalException { nit: indent 4 http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@794 PS2, Line 794: +authzProvider nit: add space around the `+` http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@812 PS2, Line 812: throws InternalException { nit: indent 4 http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@822 PS2, Line 822: +authzFactoryClassName nit: add space around the `+` -- 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: 2 Gerrit-Owner: radford nguyen <radford.ngu...@gmail.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: radford nguyen <radford.ngu...@gmail.com> Gerrit-Comment-Date: Mon, 01 Apr 2019 17:05:43 +0000 Gerrit-HasComments: Yes
