Alex Behm has posted comments on this change. Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary ......................................................................
Patch Set 4: (6 comments) Thanks, looks much nicer! http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: Line 1094: << "or execution summary."; indentation http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/client-request-state.h File be/src/service/client-request-state.h: Line 220: Status CheckProfileAccess(const std::string& user); const function? http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 636: Status ImpalaServer::QueryStateRecord::CheckProfileAccess(const string& user) { What do you think of moving this into auth-util.h like the other function so it's obvious that both implementations do the same thing? We could even consolidate the code into a single function CheckProfileAccess() that takes all 3 params, i.e. we have CheckProfileAccess(user, QueryStateRecord); CheckProfileAccess(user, ClientRequestState); CheckProfileAccess(user, effective_user, has_access); I don't fee too strongly, but seems nice to have the auth logic and error message only once. http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 639: Status CheckProfileAccess(const std::string& user); const fn http://gerrit.cloudera.org:8080/#/c/7064/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 162: private boolean enablePrivChecks_ = true; I was hoping we could remove this now because it should have the same effect as setMaskPrivChecks(null). Any reason we need to keep it? http://gerrit.cloudera.org:8080/#/c/7064/4/shell/impala_shell.py File shell/impala_shell.py: Line 515: print_to_stderr(e) are these changes for testing? -- To view, visit http://gerrit.cloudera.org:8080/7064 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
