Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12334 )

Change subject: IMPALA-8150: Fix buggy 
AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 4:

(2 comments)

Thanks for cleaning this up, and for fixing the "polarity" of the assertEquals 
statements. A few minor comments.

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java:

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@83
PS4, Line 83:     if (sentryConfig_.getConfigFile() != null) {
You've found that something prevents the file name from ever being ""? Or, that 
we want to fail the load in this case?


http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@962
PS4, Line 962:           "Authorization is enabled but the server name is null 
or empty. Set the " +
Error message says null or empty, but the code was changed to just check null...



--
To view, visit http://gerrit.cloudera.org:8080/12334
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com>
Gerrit-Comment-Date: Sat, 02 Feb 2019 03:19:23 +0000
Gerrit-HasComments: Yes

Reply via email to