Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )
Change subject: [sentry] SentryAction ...................................................................... Patch Set 7: (9 comments) Addressed the comments in https://gerrit.cloudera.org/#/c/11720/. http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@27 PS6, Line 27: replication > The word 'replication' is a bit overloaded considering its meaning for Kudu Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@28 PS6, Line 28: In this case, HiveSQL model is chosen : // to define the actions. > For those of us unfamiliar with HiveSQL, could you expand on the significan Done, though I assume you are asking about HiveSQL privilege model which is this class is partly about instead of Hive Query Language. http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@32 PS6, Line 32: // This class is not thread-safe. > I mean, sure, but it's got so little state that I don't think this warning Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@40 PS6, Line 40: the > Nit: drop 'the' Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@46 PS6, Line 46: UNINITIALIZED, > It's not clear from this patch alone, but why is it important to allow Sent Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@70 PS6, Line 70: Check if an action implies the other > Nit: Check if this action implies 'other'. Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc File src/kudu/sentry/sentry_action.cc: http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@77 PS6, Line 77: Any > Every Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@78 PS6, Line 78: CHECK(action() != Action::UNINITIALIZED); : CHECK(other.action() != Action::UNINITIALIZED); > Use CHECK_NE here. Done http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@87 PS6, Line 87: // Any action subsumes Action METADATA > Nit: terminate with period. Done -- 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: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 18 Oct 2018 05:48:01 +0000 Gerrit-HasComments: Yes
