radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )
Change subject: IMPALA-8309: add user authorization_provider flag ...................................................................... Patch Set 2: (8 comments) Addressed nits; adding e2e tests next 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 Honest question: why? I don't really see consistent usage in this file; for example `DEFINE_string(ranger_app_id` begins the description string on a newline but `DEFINE_string(server_name` begins the description on the same line... even though both description strings end up wrapping. I think the current approach is perfectly readable, so I don't understand the point of these 3 nits. 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`? Yeah that's a better name. I was waffling back and forth between using the actual `Class` object vs. the canonical name. 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() { > nit: I'm not opposed to adding the @Nullable annotation and adding `orNull` That's the thing, I wasn't able to find another example in the codebase for handling optional flags. If you could point one out I'll make sure this conforms 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 Done 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 Done 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 `+` Done 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 Done 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 `+` Done -- 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 <[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: Thu, 04 Apr 2019 02:08:02 +0000 Gerrit-HasComments: Yes
