You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colm O hEigeartaigh <co...@apache.org> on 2017/10/02 17:07:02 UTC
Code consolidation query
Hi all,
I'm preparing a patch to consolidate some duplicate code in
sentry-provider-db, and have a question:
CommandUtil.convertToTSentryPrivilege:
https://github.com/apache/sentry/blob/186e75543a23f286e24c56f73859099405b7b73a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java#L38
is identical to SentryServiceUtil.convertToTSentryPrivilege:
https://github.com/apache/sentry/blob/186e75543a23f286e24c56f73859099405b7b73a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java#L56
apart from two differences:
a) The CommandUtil version sets:
tSentryPrivilege.setAction(AccessConstants.ALL); for the
PRIVILEGE_URI_NAME case.
b) The CommandUtil version "validates" the privilege hierarchy, throwing an
exception if one of the names is not specified.
Ideally I'd like to remove CommandUtil altogether and just reference the
methods in SentryServiceUtil. Should these extra pieces also be in
SentryServiceUtil? Or is there a reason that they are different?
Colm.
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com
Re: Code consolidation query
Posted by Alexander Kolbasov <ak...@cloudera.com>.
I don't know why - I just ported this from master to sentry HA branch. I
guess we do not really support anything other then ALL on URI privileges -
there is no concept of "insert" or "select" on URI, so ALL is the only
thing that makes sense, but I don't think it is shell's business to deal
with it.
On Thu, Oct 5, 2017 at 2:06 AM, Colm O hEigeartaigh <co...@apache.org>
wrote:
> OK I can add a switch to call the validation code for the shell only. Do
> you know why the CommandUtil includes:
>
> tSentryPrivilege.setAction(AccessConstants.ALL);
>
> for the URI case? This looks a bit dodgy to me as it would override the
> value parsed via PRIVILEGE_ACTION_NAME.
>
> Colm.
>
> On Wed, Oct 4, 2017 at 11:37 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
> > Interesting - both were introduced by Colin Ma. I definitely agree that
> we
> > do not need two duplicates of the same similar functionality. The
> exception
> > part is interesting - the validator throws unchecked exceptions.
> >
> > The version in SentryServiceUtil is used only in import policy code path
> > where validation should be always ok.
> >
> > The second one is used by shell only.
> >
> > There is another internal version in SentryStore:
> >
> > private void convertToTSentryPrivilege(MSentryPrivilege
> mSentryPrivilege,
> > TSentryPrivilege privilege) {
> > privilege.setCreateTime(mSentryPrivilege.getCreateTime());
> > privilege.setAction(fromNULLCol(mSentryPrivilege.getAction()));
> > privilege.setPrivilegeScope(mSentryPrivilege.getPrivilegeScope());
> > privilege.setServerName(fromNULLCol(mSentryPrivilege.
> getServerName()));
> > privilege.setDbName(fromNULLCol(mSentryPrivilege.getDbName()));
> > privilege.setTableName(fromNULLCol(mSentryPrivilege.getTableName()));
> > privilege.setColumnName(fromNULLCol(mSentryPrivilege.
> getColumnName()));
> > privilege.setURI(fromNULLCol(mSentryPrivilege.getURI()));
> > if (mSentryPrivilege.getGrantOption() != null) {
> > privilege.setGrantOption(TSentryGrantOption.valueOf(
> mSentryPrivilege.
> > getGrantOption().toString().toUpperCase()));
> > } else {
> > privilege.setGrantOption(TSentryGrantOption.UNSET);
> > }
> > }
> >
> >
> >
> > On Mon, Oct 2, 2017 at 10:07 AM, Colm O hEigeartaigh <
> coheigea@apache.org>
> > wrote:
> >
> > > Hi all,
> > >
> > > I'm preparing a patch to consolidate some duplicate code in
> > > sentry-provider-db, and have a question:
> > >
> > > CommandUtil.convertToTSentryPrivilege:
> > >
> > > https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> > > 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> > > java/org/apache/sentry/provider/db/tools/command/
> > hive/CommandUtil.java#L38
> > >
> > > is identical to SentryServiceUtil.convertToTSentryPrivilege:
> > >
> > > https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> > > 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> > > java/org/apache/sentry/service/thrift/SentryServiceUtil.java#L56
> > >
> > > apart from two differences:
> > >
> > > a) The CommandUtil version sets:
> > > tSentryPrivilege.setAction(AccessConstants.ALL); for the
> > > PRIVILEGE_URI_NAME case.
> > > b) The CommandUtil version "validates" the privilege hierarchy,
> throwing
> > an
> > > exception if one of the names is not specified.
> > >
> > > Ideally I'd like to remove CommandUtil altogether and just reference
> the
> > > methods in SentryServiceUtil. Should these extra pieces also be in
> > > SentryServiceUtil? Or is there a reason that they are different?
> > >
> > > Colm.
> > >
> > >
> > > --
> > > Colm O hEigeartaigh
> > >
> > > Talend Community Coder
> > > http://coders.talend.com
> > >
> >
>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>
Re: Code consolidation query
Posted by Colm O hEigeartaigh <co...@apache.org>.
OK I can add a switch to call the validation code for the shell only. Do
you know why the CommandUtil includes:
tSentryPrivilege.setAction(AccessConstants.ALL);
for the URI case? This looks a bit dodgy to me as it would override the
value parsed via PRIVILEGE_ACTION_NAME.
Colm.
On Wed, Oct 4, 2017 at 11:37 PM, Alexander Kolbasov <ak...@cloudera.com>
wrote:
> Interesting - both were introduced by Colin Ma. I definitely agree that we
> do not need two duplicates of the same similar functionality. The exception
> part is interesting - the validator throws unchecked exceptions.
>
> The version in SentryServiceUtil is used only in import policy code path
> where validation should be always ok.
>
> The second one is used by shell only.
>
> There is another internal version in SentryStore:
>
> private void convertToTSentryPrivilege(MSentryPrivilege mSentryPrivilege,
> TSentryPrivilege privilege) {
> privilege.setCreateTime(mSentryPrivilege.getCreateTime());
> privilege.setAction(fromNULLCol(mSentryPrivilege.getAction()));
> privilege.setPrivilegeScope(mSentryPrivilege.getPrivilegeScope());
> privilege.setServerName(fromNULLCol(mSentryPrivilege.getServerName()));
> privilege.setDbName(fromNULLCol(mSentryPrivilege.getDbName()));
> privilege.setTableName(fromNULLCol(mSentryPrivilege.getTableName()));
> privilege.setColumnName(fromNULLCol(mSentryPrivilege.getColumnName()));
> privilege.setURI(fromNULLCol(mSentryPrivilege.getURI()));
> if (mSentryPrivilege.getGrantOption() != null) {
> privilege.setGrantOption(TSentryGrantOption.valueOf(mSentryPrivilege.
> getGrantOption().toString().toUpperCase()));
> } else {
> privilege.setGrantOption(TSentryGrantOption.UNSET);
> }
> }
>
>
>
> On Mon, Oct 2, 2017 at 10:07 AM, Colm O hEigeartaigh <co...@apache.org>
> wrote:
>
> > Hi all,
> >
> > I'm preparing a patch to consolidate some duplicate code in
> > sentry-provider-db, and have a question:
> >
> > CommandUtil.convertToTSentryPrivilege:
> >
> > https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> > 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> > java/org/apache/sentry/provider/db/tools/command/
> hive/CommandUtil.java#L38
> >
> > is identical to SentryServiceUtil.convertToTSentryPrivilege:
> >
> > https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> > 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> > java/org/apache/sentry/service/thrift/SentryServiceUtil.java#L56
> >
> > apart from two differences:
> >
> > a) The CommandUtil version sets:
> > tSentryPrivilege.setAction(AccessConstants.ALL); for the
> > PRIVILEGE_URI_NAME case.
> > b) The CommandUtil version "validates" the privilege hierarchy, throwing
> an
> > exception if one of the names is not specified.
> >
> > Ideally I'd like to remove CommandUtil altogether and just reference the
> > methods in SentryServiceUtil. Should these extra pieces also be in
> > SentryServiceUtil? Or is there a reason that they are different?
> >
> > Colm.
> >
> >
> > --
> > Colm O hEigeartaigh
> >
> > Talend Community Coder
> > http://coders.talend.com
> >
>
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com
Re: Code consolidation query
Posted by Alexander Kolbasov <ak...@cloudera.com>.
Interesting - both were introduced by Colin Ma. I definitely agree that we
do not need two duplicates of the same similar functionality. The exception
part is interesting - the validator throws unchecked exceptions.
The version in SentryServiceUtil is used only in import policy code path
where validation should be always ok.
The second one is used by shell only.
There is another internal version in SentryStore:
private void convertToTSentryPrivilege(MSentryPrivilege mSentryPrivilege,
TSentryPrivilege privilege) {
privilege.setCreateTime(mSentryPrivilege.getCreateTime());
privilege.setAction(fromNULLCol(mSentryPrivilege.getAction()));
privilege.setPrivilegeScope(mSentryPrivilege.getPrivilegeScope());
privilege.setServerName(fromNULLCol(mSentryPrivilege.getServerName()));
privilege.setDbName(fromNULLCol(mSentryPrivilege.getDbName()));
privilege.setTableName(fromNULLCol(mSentryPrivilege.getTableName()));
privilege.setColumnName(fromNULLCol(mSentryPrivilege.getColumnName()));
privilege.setURI(fromNULLCol(mSentryPrivilege.getURI()));
if (mSentryPrivilege.getGrantOption() != null) {
privilege.setGrantOption(TSentryGrantOption.valueOf(mSentryPrivilege.getGrantOption().toString().toUpperCase()));
} else {
privilege.setGrantOption(TSentryGrantOption.UNSET);
}
}
On Mon, Oct 2, 2017 at 10:07 AM, Colm O hEigeartaigh <co...@apache.org>
wrote:
> Hi all,
>
> I'm preparing a patch to consolidate some duplicate code in
> sentry-provider-db, and have a question:
>
> CommandUtil.convertToTSentryPrivilege:
>
> https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java#L38
>
> is identical to SentryServiceUtil.convertToTSentryPrivilege:
>
> https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> java/org/apache/sentry/service/thrift/SentryServiceUtil.java#L56
>
> apart from two differences:
>
> a) The CommandUtil version sets:
> tSentryPrivilege.setAction(AccessConstants.ALL); for the
> PRIVILEGE_URI_NAME case.
> b) The CommandUtil version "validates" the privilege hierarchy, throwing an
> exception if one of the names is not specified.
>
> Ideally I'd like to remove CommandUtil altogether and just reference the
> methods in SentryServiceUtil. Should these extra pieces also be in
> SentryServiceUtil? Or is there a reason that they are different?
>
> Colm.
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>