Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13069 )
Change subject: [authz] new SentryAuthzProvider's caching strategy ...................................................................... Patch Set 7: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/13069/7/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/7/src/kudu/master/sentry_privileges_fetcher.cc@534 PS7, Line 534: t expected to receive any requests nit: "receive any requests" makes it sound like this is a service and that the requests are external. Maybe reword this as, "SentryPrivilegesFetcher is not expected to request authorizables of the SERVER scope unless this method is called from test code." Also, following our in-person chat about introducing a flag to gate this, I realized we actually already have a flag for checking if we're in a test: IsGTest() We using sparingly though because it's kind of ugly and doesn't add a whole lot as test coverage goes IMO http://gerrit.cloudera.org:8080/#/c/13069/7/src/kudu/master/sentry_privileges_fetcher.cc@538 PS7, Line 538: received request to fetch privileges of the SERVER scope " : "on authorizable '$0' for user '$1'", table_name, user); nit: same here re "received request", maybe reword as "requesting to fetch privileges of the SERVER scope on..." -- To view, visit http://gerrit.cloudera.org:8080/13069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id96181345e357a104e28314d8d8d88633dcf9608 Gerrit-Change-Number: 13069 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 24 Apr 2019 03:16:55 +0000 Gerrit-HasComments: Yes
