Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 7: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92 PS6, Line 92: const std::string& user, > Done Nit: there's a subtle difference between 'required_grant_option' and 'require_grant_option': the former suggests that the "grant option" is always required, but that its value should be either true or false as dictated by the parameter, while the latter more clearly (IMHO) indicates that a grant option is either required or it is not. Put another way, "require_grant_option" more clearly maps to the English sentence "do we require a GRANT OPTION?" while "required_grant_option" is more ambiguous, and it wouldn't be unheard of for it to map to something like "the required grant option should have a value of false". http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@176 PS7, Line 176: DCHECK(scope != SentryAuthorizableScope::Scope::COLUMN); Use DCHECK_NE instead. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@321 PS7, Line 321: WARN_NOT_OK(s, Substitute("failed to construct sentry privilege scope from $0", Nit: for clarity via symmetry with L315, could you break out the s.ok() check from L325 and make a separate "if not ok, continue" check here? -- To view, visit http://gerrit.cloudera.org:8080/12500 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a Gerrit-Change-Number: 12500 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <[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: Tue, 12 Mar 2019 18:05:58 +0000 Gerrit-HasComments: Yes
