Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21026 )
Change subject: IMPALA-12609: Implement SHOW METADATA TABLES IN statement to list Iceberg Metadata tables ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/21026/6/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@1261 PS6, Line 1261: // Test that even if we have privileges on a different table in the same db, we : // still can't access 'iceberg_query_metadata' if we don't > Added a comment to clarify. thanks! http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py@129 PS6, Line 129: # Make sure there is no privilege stuck from the previous execution of the test. : admin_client.execute("revoke {priv} on database {db} from user {user}".format( : priv=priv, db=unique_db, user=user), user=ADMIN) > Interestingly if I delete the database but do not drop the privileges next I think if we can't justify the existence of some code then that code shouldn't exist. What I can think of is that when that other test was being written in some intermediate state there were test issues resulting that the DB or the privilege remained and hence this extra safety net. I don't think that this can happen with this test. I saw in other tests that creating the DB is in an outer try-finally block while granting the privileges is in an internal try-finally (I recall I also wrote something similar not that long ago). Note that dropping the DB will not drop the privileges associated with it so once you recreate the DB with the same name the old privileges will be there. Hence the need for an extra step to drop the privileges too (from Ranger). http://gerrit.cloudera.org:8080/#/c/21026/6/tests/authorization/test_authorization.py@137 PS6, Line 137: assert str(exc1).strip() + non_existent_suffix == str(exc2).strip() > The error message contains the username (which depends on where the test is We can then assert on "AuthorizationException" and "does not have privileges to access" are part of the error message. -- To view, visit http://gerrit.cloudera.org:8080/21026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide10ccf10fc0abf5c270119ba7092c67e712ec49 Gerrit-Change-Number: 21026 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 22 Feb 2024 16:15:57 +0000 Gerrit-HasComments: Yes
