Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )
Change subject: [sentry] SentryAction ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action-test.cc File src/kudu/sentry/sentry_action-test.cc: http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action-test.cc@53 PS5, Line 53: unordered_set<SentryAction*> actions; This would be simpler as a vector<SentryAction>, since it could use an intializer list to populate instead of having to create and insert the pointers. You could do the METADATA loop first, then add metadata and do theother loop. http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.h@76 PS5, Line 76: bool Imply(const SentryAction& other); Can this be const? http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.cc File src/kudu/sentry/sentry_action.cc: http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.cc@77 PS5, Line 77: if (action() == Action::UNINITIALIZED) { Maybe this should CHECK fail? It should be a bug if an uninitialized action is used, right? In either case it should probably check both this and other. -- To view, visit http://gerrit.cloudera.org:8080/11656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49 Gerrit-Change-Number: 11656 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 15 Oct 2018 23:42:24 +0000 Gerrit-HasComments: Yes
