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 9: (19 comments) http://gerrit.cloudera.org:8080/#/c/12901/9/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/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141 PS9, Line 141: getAuthorizationFactoryClassOrNull > nit: remove the `orNull` in the function name because the annotation is already present or because you don't like the name? i think there should be some indication that the return value can be null, if we're not using Optional http://gerrit.cloudera.org:8080/#/c/12901/9/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/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java@114 PS9, Line 114: //<<<<<<< 67f77d41d40523074385b8dbccfa6ef6ef81dd57 > Why is this commented out? ugh was sanity checking a merge conflict and forgot to delete http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py File tests/authorization/test_provider.py: http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@21 PS5, Line 21: import os > flake8: F401 'time' imported but unused Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@25 PS5, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite > flake8: F401 'tests.util.filesystem_utils.IS_LOCAL' imported but unused Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@28 PS5, Line 28: class TestProviderFails(CustomClusterTestSuite): > flake8: F401 'tests.common.impala_test_suite.ImpalaTestSuite' imported but Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@38 PS5, Line 38: LOG_DIR = tempfile.mkdtemp(prefix="test_provider_", dir=os.getenv("LOG_DIR")) > line has trailing whitespace Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@39 PS5, Line 39: > flake8: E302 expected 2 blank lines, found 1 Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@53 PS5, Line 53: e > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@54 PS5, Line 54: > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@55 PS5, Line 55: a > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@58 PS5, Line 58: > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@59 PS5, Line 59: > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@60 PS5, Line 60: > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@66 PS5, Line 66: > flake8: E502 the backslash is redundant between brackets Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@67 PS5, Line 67: > flake8: E501 line too long (92 > 90 characters) Ack http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@68 PS5, Line 68: > flake8: E124 closing bracket does not match visual indentation Ack http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py File tests/authorization/test_provider.py: http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@60 PS6, Line 60: not the happiest with this approach; open to suggestions http://gerrit.cloudera.org:8080/#/c/12901/7/tests/authorization/test_provider.py File tests/authorization/test_provider.py: http://gerrit.cloudera.org:8080/#/c/12901/7/tests/authorization/test_provider.py@27 PS7, Line 27: > flake8: E302 expected 2 blank lines, found 1 Ack http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py@38 PS4, Line 38: """ > Would prefer if there was an e2e test with an invalid as well as valid `--a Ack -- 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: 9 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, 22 Apr 2019 20:19:57 +0000 Gerrit-HasComments: Yes
