Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13601 )
Change subject: IMPALA-8658: Populate missing Ranger audit fields ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/13601/3/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/13601/3/common/thrift/JniCatalog.thrift@642 PS3, Line 642: 6: required string sql_stmt Can you name this 'redacted_sql_stmt' or something so that it's clear this is supposed to be post-redaction? http://gerrit.cloudera.org:8080/#/c/13601/3/common/thrift/JniCatalog.thrift@640 PS3, Line 640: : // The SQL statement to be logged. : 6: required string sql_stmt : : // The client IP address. : 7: required string client_ip What do you think about moving these to TCatalogServiceRequestHeader? They seem generally useful for logging/tracing what's going on on the catalogd for any operation, and 'client_ip' certainly seems to fit nicely with 'requesting_user' which is already present there. http://gerrit.cloudera.org:8080/#/c/13601/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java File fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java: http://gerrit.cloudera.org:8080/#/c/13601/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java@111 PS3, Line 111: } catch (UnknownHostException e) { : throw new AnalysisException("Unable to get the client IP address", e); seems weird that this can throw. Is it trying to reverse-lookup or something? http://gerrit.cloudera.org:8080/#/c/13601/3/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/13601/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@222 PS3, Line 222: } catch (UnknownHostException e) { : throw new InternalException("Unable to get the client IP address", e); : } similar to elsewhere, it seems like this should never happen if we just have IP addresses and not names. Perhaps we can catch it and throw as RuntimeException in getClientIpAddress()? Or we could use a different way of getting it that can't throw? http://gerrit.cloudera.org:8080/#/c/13601/3/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/3/fe/src/main/java/org/apache/impala/util/TSessionStateUtil.java@47 PS3, Line 47: return InetAddress.getByName(session.getNetwork_address().getHostname()) We could avoid the 'throws UnknownHostException' by just using String.format("%s:%d", ...) with the host and port, no? -- 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: 3 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: Thu, 13 Jun 2019 20:03:09 +0000 Gerrit-HasComments: Yes
