impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5582: Store sentry privileges in lower case
Date Wed, 12 Jul 2017 20:32:36 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5582: Store sentry privileges in lower case

Patch Set 2:

(1 comment)

I think we need to figure out a way to test this. A test could be added to
which includes a delay. This might be a good time to expose a way to configure the Sentry
polling period, see

    // Sentry Service is enabled.
    // TODO: Make this configurable
    policyReader_.scheduleAtFixedRate(new PolicyReader(), 0, 60,
File fe/src/main/java/org/apache/impala/analysis/

PS2, Line 132:     if (dbName_ != null) privilege.setDb_name(dbName_.toLowerCase());
             :     if (tableName_ != null) privilege.setTable_name(tableName_.getTbl().toLowerCase());
             :     if (uri_ != null) privilege.setUri(uri_.toString());
             :     if (columnName != null) privilege.setColumn_name(columnName.toLowerCase());
This seems like it could be a brittle fix, because it looks like the relevant check for removal
upon the Sentry policy update thread is checking the "privilege name" (
- see below). So if we ever change what gets stored in the "privilege name" (e.g. adding a
field) or create TPrivileges in some other way, then we can have this issue again. I think
it probably makes sense to change RolePrivilege.buildRolePrivilegeName() to lower case the
returned privilege name.

            // Assume all privileges should be removed. Privileges that still exist are
            // deleted from this set and we are left with the set of privileges that need
            // to be removed.
            Set<String> privilegesToRemove = role.getPrivilegeNames();

            // Check all the privileges that are part of this role.
            for (TSentryPrivilege sentryPriv:
                sentryPolicyService_.listRolePrivileges(processUser_, role.getName())) {
              TPrivilege thriftPriv =

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide3dfa601fcf77f5acc6adce9bea443aea600901
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: anujphadke <>
Gerrit-HasComments: Yes

View raw message