Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15854 )
Change subject: IMPALA-9597: Eliminate redundant Ranger audits when a query involves column masking ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/15854/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15854/1//COMMIT_MSG@7 PS1, Line 7: when a query involves nit: the title could be made shorter by replacing "when a query involves" with "for" http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@74 PS1, Line 74: authzExceptionIsNull nit, naming: I would prefer something that refers the situation that occurred, not a language construct like Exception - e.g. 'authzError' or 'unsuccessfulAuthz' http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@360 PS1, Line 360: Preconditions.checkArgument(authzCtx instanceof RangerAuthorizationContext); I comment like "We assume that all events for column masking related until now" would be helpful. http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java: http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java@55 PS1, Line 55: deduplicateAuthzEvents I think that the most important thing is not deduplication, but the "stashing" of authz events so far, so that we add them to another list and clear the one in auditHandler_. I would prefer to separate these functionalities - e.g. create a stashAuditEvents(), a deduplicateStashedAuditEvents() and an applyStashedAuditEvents() function. Deduplication is only needed if the authorization was successful. I would be also nice to purpose if this in a comment somewhere - we want to change the order masking and not masking related events, and deduplicate only the former. http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java: http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@358 PS1, Line 358: "@column", "select", "functional/alltypestiny/id just curious: in the authzOk case, the first event was a table level select, then we checked all referenced columns.Here the first event is one of the columns. Do you know the reason for the difference? http://gerrit.cloudera.org:8080/#/c/15854/1/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@378 PS1, Line 378: // Fang-Yu: : List<AuthzAuditEvent> events = rangerCtx.getAuditHandler().getAuthzEvents(); Did this remain here by mistake? -- To view, visit http://gerrit.cloudera.org:8080/15854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42d60130fba93d63fbc36949f2bf746b7ae2497d Gerrit-Change-Number: 15854 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 05 May 2020 16:57:25 +0000 Gerrit-HasComments: Yes
