Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11880 )
Change subject: IMPALA-7764: Improve SentryProxy test coverage ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java File fe/src/test/java/org/apache/impala/util/SentryProxyTest.java: http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@94 PS3, Line 94: nit: use consistent spacing for these. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@100 PS3, Line 100: false this (resetVersions) seems to have caused issues yet all of the tests here consider only the false case. worth adding tests when set to true? from looking at that code, checking that updates get a version change (on reset) would test priv name construction and its lookup at the least. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@242 PS3, Line 242: mixedCaseRoleName what's the point of this one? http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@261 PS3, Line 261: lowerCaseRoleName.toUpperCase() just use the upperCaseRoleName? http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@266 PS3, Line 266: SentryProxy.refreshSentryAuthorization(catalog, sentryService_, USER, false); what is the purpose of this refresh? the operation before is expected to fail (L261). http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@291 PS3, Line 291: "functional_kudu" these are all different users, so why the different privs? http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@295 PS3, Line 295: Impala stores the role name in case insensitive way. expecting this to be about users. -- To view, visit http://gerrit.cloudera.org:8080/11880 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1190638283f051cc37fda7ea5dd0c5e8a70d40db Gerrit-Change-Number: 11880 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 09 Nov 2018 00:28:27 +0000 Gerrit-HasComments: Yes
