Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12897 )
Change subject: wip sentry: generate authz tokens ...................................................................... Patch Set 1: (7 comments) left a few comments skimming through http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/gutil/map-util.h File src/kudu/gutil/map-util.h: http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/gutil/map-util.h@841 PS1, Line 841: std::move(entry.second) Interesting, I didn't not expect it would allow to use std::move(entry.second) while entry is a const references. http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/catalog_manager.cc@2752 PS1, Line 2752: token_signer && user Is it valid to have situations when { token_signer != nullptr, user == boost::none } or { token_signed == nullptr, user != boost::none } ? http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@43 PS1, Line 43: struct SentryPrivilege { Add a few lines of comments explaining why this structure is useful. http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@47 PS1, Line 47: boost::optional<std::string> column) nit: for convenience, maybe make last two parameters boost::none by default? http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@52 PS1, Line 52: #ifndef NDEBUG What if set 'scope' to DATABASE and specify db, table, and column as well? Will it be a valid structure? http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@70 PS1, Line 70: sentry::SentryAuthorizableScope::Scope scope; Is it necessary to store the scope once the structure is constructed? If it's really so, why not to make it const? http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/sentry/sentry_action.h@a50 PS1, Line 50: Instead, is it possible to update FixedBitSet to work with enums like this as well? -- To view, visit http://gerrit.cloudera.org:8080/12897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I052432408045c48f6fe9bf921fd3cb6bcc36e9ad Gerrit-Change-Number: 12897 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 01 Apr 2019 22:46:29 +0000 Gerrit-HasComments: Yes