impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary
Date Fri, 02 Jun 2017 22:13:29 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary

Patch Set 1:


Still trying to understand all the different paths where the profile can be fetched. Here
are a few comments to start with.
File be/src/service/

Line 66: static const string LIMITED_PROFILE_INFO_STRINGS[] = {"Session ID", "Session Type",
Will this show the timeline?
File be/src/service/client-request-state.h:

Line 195:   const std::string limited_profile_str() const;
Why const std::string?
File be/src/service/impala-server.h:

Line 386:     const std::string& GetEffectiveUser() {
Consider changing this function to

std::string GetEffectiveUser(SessionState);

and move it into auth-util.h/cc

That way it's obvious whether the two implementations do the same thing.

Line 480:   /// If the user asking for this profile is the same user that run the query
.. that runs the query ...

Line 489:   /// query profile. Otherwise, this function returns an empty exec summary.
Seems better to return an auth warning string instead of an empty string.
File common/thrift/ImpalaInternalService.thrift:

Line 362:   // profile.
give an example why this may be the case
File fe/src/main/java/org/apache/impala/analysis/

Line 489:     // Check any masked requests. If the masked requests have an associated error
Check all masked requests. If a masked request has an associated...

Line 492:     // These checks don't result in an Authorization exception but set the

Line 494:     //
extra line
File fe/src/main/java/org/apache/impala/analysis/

Line 2533:     if (!enablePrivChecks_) {
This new logic and naming could use a little cleanup. How about we rename enablePrivChecks_
to maskPrivChecks_ and introduce a new Analyzer.setMaskPrivChecks(String msg) and Analyzer.unsetMaskPrivChecks()
(or something similar).

That way when masking is enabled we always add a priv req to the masked list together with
the user-provided 'msg' which controls whether a failure to auth that masked priv request
leads to an AuthException (the 'msg' could be null of course).

Line 2534:       globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, ""));
I'd prefer null instead of an empty string. Seems clearer what it means.

Line 2538:     if (Strings.isNullOrEmpty(authErrorMsg_)) {
Change this to authErrorMsg_ != null? Treating the empty string and null the same way seems
error prone.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-HasComments: Yes

View raw message