Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13601 )
Change subject: IMPALA-8658: Populate missing Ranger audit fields ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/13601/5/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/13601/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@43 PS5, Line 43: * @param sqlStmt the SQL statement to be logged for auditing > nit: add a @param for sessionState? Done http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java: http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java@59 PS5, Line 59: sqlStmt_ = Preconditions.checkNotNull(sqlStmt); > should you Preconditions.checkNotNull() for these? or Objects.firstNonNull( Done http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@367 PS5, Line 367: ddlRequest.getHeader(), ddlRequest.g > it seems like 'requestingUser' always is being constructed out of ddlReques Done http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@367 PS5, Line 367: ddlRequest.getHeader(), ddlRequest.g > it seems like 'requestingUser' always is being constructed out of ddlReques Done http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/util/TSessionStateUtil.java File fe/src/main/java/org/apache/impala/util/TSessionStateUtil.java: http://gerrit.cloudera.org:8080/#/c/13601/5/fe/src/main/java/org/apache/impala/util/TSessionStateUtil.java@44 PS5, Line 44: InetAddresses.forString(session.getNetwork_address().getHostname()) : .getHostAddress(); > hrm, now I'm wondering why we can't just use getNetowkr_address().getHostna Ranger wants it into the IP address form. It maybe safe to provide Ranger what it needs in case it needs to do a reverse lookup or something. -- To view, visit http://gerrit.cloudera.org:8080/13601 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I167bc35411ad9b4164c292077ff082671967cff8 Gerrit-Change-Number: 13601 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 17 Jun 2019 19:01:42 +0000 Gerrit-HasComments: Yes
