Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )
Change subject: [sentry] SentryAction ...................................................................... Patch Set 7: (9 comments) Missed this one too. 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. Perhaps "carbon copy"? Or "C++ implementation of the SentryAction Java class" (if that's what it is)? 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 significance of this and provide a link for more information? 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 is that interesting. You could even make it thread-safe with a minimum of fuss, by converting FromString into a static factory method and removing the default constructor. Then action_ will be immutable. http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@40 PS6, Line 40: the Nit: drop 'the' 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 SentryAction to be uninitialized? If you convert FromString() into a static factory method and remove the default constructor, it won't be possible to create a SentryAction without a valid Action. 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'. 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 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. 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. -- 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 Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 17 Oct 2018 03:41:10 +0000 Gerrit-HasComments: Yes
