Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for visibility ...................................................................... Patch Set 3: (8 comments) Thanks for making the suggested change. I have some minor suggestions below and its good to go from my side. 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: visibility_privilege_set may be rename to min_privilege_set_for_show_stmts? http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@283 PS3, Line 283: Any o Any one of them.. http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@284 PS3, Line 284: "table. Default to \"any\" which means if the user has any privilege (ALL, SELECT, " s/Default/Defaults http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@288 PS3, Line 288: practise s/practise/practice http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@291 PS3, Line 291: nit, additional space http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@292 PS3, Line 292: Because a lot of privilege checks on invisible dbs/tables can " : "be bypassed I think this is redundant information and can be skipped. 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. 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("Ignored illegal privilege name '{}'", pStr, e); Do you think we should throw ImpalaException here instead of silently logging ignoring the user configuration? -- 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: 3 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 18 Dec 2019 22:55:45 +0000 Gerrit-HasComments: Yes
