Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility ...................................................................... Patch Set 4: (10 comments) Thanks for the review! Rename the flag and throw exception for illegal flag value. http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@282 PS3, Line 282: min_privilege_set_for_sh > may be rename to min_privilege_set_for_show_stmts? Done. Feel hard to find a good name for this... http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@283 PS3, Line 283: Any o > Any one of them.. Done http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@284 PS3, Line 284: "or table. Defaults to \"any\" which means if the user has any privilege (CREATE, " > s/Default/Defaults Done http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@288 PS3, Line 288: be shown > s/practise/practice Done http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@291 PS3, Line 291: ec > nit, additional space Done http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@292 PS3, Line 292: e of privileges. No significant performance gain when using " : "Ranger"); > I think this is redundant information and can be skipped. Done http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@93 PS3, Line 93: hasAccess > nit, not a blocker for this patch. I think sentry already does this for us. Sorry, I'm not quite clear... This is the base class for both Sentry and Ranger authorization. Do you mean we can override the 'hasAnyAccess' function in SentryAuthorizationChecker using Sentry's API directly? http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/service/Frontend.java@326 PS3, Line 326: LOG.error("Illegal privilege name '{}'", pStr, e); > Do you think we should throw ImpalaException here instead of silently loggi Yes, that's better for careless users that even don't read logs. http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py@574 PS2, Line 574: > flake8: E131 continuation line unaligned for hanging indent Done http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py@600 PS2, Line 600: > flake8: E131 continuation line unaligned for hanging indent Done -- To view, visit http://gerrit.cloudera.org:8080/14904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609 Gerrit-Change-Number: 14904 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Thu, 19 Dec 2019 02:09:03 +0000 Gerrit-HasComments: Yes
