Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11880 )
Change subject: IMPALA-7764: Improve SentryProxy test coverage ...................................................................... Patch Set 5: (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: l > nit: use consistent spacing for these. Done http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@100 PS3, Line 100: > this (resetVersions) seems to have caused issues yet all of the tests here Yeah good idea. Tests updated. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@242 PS3, Line 242: server=server1->d > what's the point of this one? It's just a good additional test case usually when dealing with case. I can remove it if you think it's not necessary. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@261 PS3, Line 261: > just use the upperCaseRoleName? Oops. Yeah this can just be upperCaseRoleName. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@266 PS3, Line 266: } > what is the purpose of this refresh? the operation before is expected to fa Yeah I guess you're right. Done. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@291 PS3, Line 291: > these are all different users, so why the different privs? If we just used one privilege, we wouldn't know if the user was assigned to the right privilege. There could be a bug where there was only one privilege and it was used by 3 different users as opposed to having 3 privileges (even though they are the same). Having 3 separate privileges ensures that there are 3 different privileges stored in the catalog. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@295 PS3, Line 295: atch (Exception e) { > expecting this to be about users. Typo. Done. -- 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: 5 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 15:37:14 +0000 Gerrit-HasComments: Yes
