You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li <li...@cloudera.com> on 2017/07/14 22:59:23 UTC
Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java a9d05b1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java 2095469
Diff: https://reviews.apache.org/r/60883/diff/1/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
> On July 17, 2017, 4:51 p.m., Brian Towles wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Line 214 (original), 225 (patched)
> > <https://reviews.apache.org/r/60883/diff/1/?file=1777189#file1777189line226>
> >
> > Needed for public function? or should this be a public function?
It has to be public function, so in org.apache.sentry.provider.db.service.persistent.TestHMSFollowerSentryStoreIntegration, I can call it.
SentryStore and HmsFollower are in different folder, so some functions have to be public.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180688
-----------------------------------------------------------
On July 17, 2017, 9:04 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 17, 2017, 9:04 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java a9d05b1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java 2095469
>
>
> Diff: https://reviews.apache.org/r/60883/diff/2/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
> On July 17, 2017, 4:51 p.m., Brian Towles wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Line 214 (original), 225 (patched)
> > <https://reviews.apache.org/r/60883/diff/1/?file=1777189#file1777189line226>
> >
> > Needed for public function? or should this be a public function?
>
> Na Li wrote:
> It has to be public function, so in org.apache.sentry.provider.db.service.persistent.TestHMSFollowerSentryStoreIntegration, I can call it.
> SentryStore and HmsFollower are in different folder, so some functions have to be public.
>
> Brian Towles wrote:
> If its public then do we need the @VisibleForTesting was more my point
I will remove the annotation.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180688
-----------------------------------------------------------
On July 17, 2017, 9:04 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 17, 2017, 9:04 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java a9d05b1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java 2095469
>
>
> Diff: https://reviews.apache.org/r/60883/diff/2/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Brian Towles <bt...@cloudera.com>.
> On July 17, 2017, 11:51 a.m., Brian Towles wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Line 214 (original), 225 (patched)
> > <https://reviews.apache.org/r/60883/diff/1/?file=1777189#file1777189line226>
> >
> > Needed for public function? or should this be a public function?
>
> Na Li wrote:
> It has to be public function, so in org.apache.sentry.provider.db.service.persistent.TestHMSFollowerSentryStoreIntegration, I can call it.
> SentryStore and HmsFollower are in different folder, so some functions have to be public.
If its public then do we need the @VisibleForTesting was more my point
- Brian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180688
-----------------------------------------------------------
On July 17, 2017, 4:04 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 17, 2017, 4:04 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java a9d05b1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java 2095469
>
>
> Diff: https://reviews.apache.org/r/60883/diff/2/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Brian Towles <bt...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180688
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
Lines 99-112 (original), 99-112 (patched)
<https://reviews.apache.org/r/60883/#comment255919>
Can we move "server1" to be a const in HiveAuthzConf to have a single place for "default" name change
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 81-84 (original), 82-89 (patched)
<https://reviews.apache.org/r/60883/#comment255916>
Can we clean this up to not setting the same authServerName var while nested.
maybe
```java
if (authServerName == null) {
authServerName = conf.get(HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME.getVar(),
conf.get(HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED.getVar(),
AuthzConf ars.AUTHZ_SERVER_NAME_DEPRECATED.getDefault());
}
```
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 134 (patched)
<https://reviews.apache.org/r/60883/#comment255917>
Lower case first letter of function name getAuthServerName
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Line 214 (original), 225 (patched)
<https://reviews.apache.org/r/60883/#comment255918>
Needed for public function? or should this be a public function?
- Brian Towles
On July 14, 2017, 5:59 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 5:59 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java a9d05b1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java 2095469
>
>
> Diff: https://reviews.apache.org/r/60883/diff/1/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
> On July 18, 2017, 8:49 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
> > Line 99 (original), 100 (patched)
> > <https://reviews.apache.org/r/60883/diff/3/?file=1778709#file1778709line100>
> >
> > Why are we changing the the defualt value?
> >
> > It might cause issues which we can foresee.
> > Unless we have specific reson, i feel we need not change it.
I revert the change. Now default remains the same
> On July 18, 2017, 8:49 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 82 (original), 84 (patched)
> > <https://reviews.apache.org/r/60883/diff/3/?file=1778711#file1778711line85>
> >
> > As you have explicitly imported "org.apache.sentry.binding.hive.conf.HiveAuthzConf.AuthzConfVars" you can use AuthzConfVars directly.
done
> On July 18, 2017, 8:49 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 83 (original), 85 (patched)
> > <https://reviews.apache.org/r/60883/diff/3/?file=1778711#file1778711line86>
> >
> > updating authServerName(arguemnt) provided is confusing. why not update the member variable directly?
I tried to update the member variable after it is assigned input value. I have error since the member variable is final. So I keep it unchanged. Are you OK with that?
> On July 18, 2017, 8:49 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
> > Lines 185 (patched)
> > <https://reviews.apache.org/r/60883/diff/3/?file=1778713#file1778713line185>
> >
> > Please update the tests methods with javadocs explaining what these tests are really doing.
done
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180861
-----------------------------------------------------------
On July 19, 2017, 10:22 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 19, 2017, 10:22 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 083ee4c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d67c162
>
>
> Diff: https://reviews.apache.org/r/60883/diff/4/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On July 18, 2017, 8:49 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 83 (original), 85 (patched)
> > <https://reviews.apache.org/r/60883/diff/3/?file=1778711#file1778711line86>
> >
> > updating authServerName(arguemnt) provided is confusing. why not update the member variable directly?
>
> Na Li wrote:
> I tried to update the member variable after it is assigned input value. I have error since the member variable is final. So I keep it unchanged. Are you OK with that?
variable was made final as it was not updated. If you have to update it feel free to do so by making it non final.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180861
-----------------------------------------------------------
On July 21, 2017, 2:30 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 21, 2017, 2:30 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
>
>
> Diff: https://reviews.apache.org/r/60883/diff/5/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review180861
-----------------------------------------------------------
I still need to have a closer loof at the tests.
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
Line 99 (original), 100 (patched)
<https://reviews.apache.org/r/60883/#comment256165>
Why are we changing the the defualt value?
It might cause issues which we can foresee.
Unless we have specific reson, i feel we need not change it.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 82 (original), 84 (patched)
<https://reviews.apache.org/r/60883/#comment256168>
As you have explicitly imported "org.apache.sentry.binding.hive.conf.HiveAuthzConf.AuthzConfVars" you can use AuthzConfVars directly.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 83 (original), 85 (patched)
<https://reviews.apache.org/r/60883/#comment256169>
updating authServerName(arguemnt) provided is confusing. why not update the member variable directly?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
Lines 185 (patched)
<https://reviews.apache.org/r/60883/#comment256170>
Please update the tests methods with javadocs explaining what these tests are really doing.
- kalyan kumar kalvagadda
On July 18, 2017, 7:06 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 18, 2017, 7:06 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 083ee4c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d67c162
>
>
> Diff: https://reviews.apache.org/r/60883/diff/3/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
(Updated July 21, 2017, 2:30 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
Diff: https://reviews.apache.org/r/60883/diff/5/
Changes: https://reviews.apache.org/r/60883/diff/4-5/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
> > Lines 90 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780704#file1780704line90>
> >
> > This file has only cosmetic changes - do we really need these changes? Less files to touch.
It is not just cosmetic changes. It makes sure the default values for AUTHZ_SERVER_NAME and AUTHZ_SERVER_NAME_DEPRECATED are the same
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java
> > Line 54 (original), 54 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780705#file1780705line54>
> >
> > Why this change? The original comment was reasonable and matches the code.
no need to change this. I will revert it
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780706#file1780706line52>
> >
> > This field isn't really used for anything, just for tests which can easily extract the value from NotificationProcessor.
will change
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 76 (original), 76 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780706#file1780706line79>
> >
> > Should it really be public?
Yes. TestHMSFollowerSentryStoreIntegration is under org.apache.sentry.provider.db.service.persistent (so I don't need to change SentryStore functions to public), and HMSFollower is under org.apache.sentry.service.thrift
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 304 (original), 312 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780706#file1780706line315>
> >
> > Why is this public?
save reason above. It is made to be public for testing purpose. Brian asks me to remove @VisibleForTesting since it is public.
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
> > Line 59 (original), 59 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780707#file1780707line59>
> >
> > Why this change? Ommitting public works fine.
If I remove "public", I will get this error "/home/lina/sw/sentry/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java:39: error: HiveSimpleConnectionFactory is not public in org.apache.sentry.service.thrift; cannot be accessed from outside package"
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
> > Lines 19 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780708#file1780708line19>
> >
> > Why this test lives here rather then in the same package as HMSFollower?
It tests integration of HMSFollower and SentryStore. If I put it under the same package as HMSFollower, I need to change a lot of functions in SentryStore to be public to setup the tests.
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
> > Lines 213 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780709#file1780709line213>
> >
> > Do you need @Test here?
> >
> > Please document what this test case is testing
Yes. Will add
> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
> > Lines 223 (patched)
> > <https://reviews.apache.org/r/60883/diff/5/?file=1780709#file1780709line223>
> >
> > Please document what this case is testing. Same for tests below.
will do
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review181118
-----------------------------------------------------------
On July 21, 2017, 2:30 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 21, 2017, 2:30 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
>
>
> Diff: https://reviews.apache.org/r/60883/diff/5/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review181118
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
Lines 90 (patched)
<https://reviews.apache.org/r/60883/#comment256536>
This file has only cosmetic changes - do we really need these changes? Less files to touch.
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java
Line 54 (original), 54 (patched)
<https://reviews.apache.org/r/60883/#comment256537>
Why this change? The original comment was reasonable and matches the code.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 49 (patched)
<https://reviews.apache.org/r/60883/#comment256545>
This field isn't really used for anything, just for tests which can easily extract the value from NotificationProcessor.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 76 (original), 76 (patched)
<https://reviews.apache.org/r/60883/#comment256539>
Should it really be public?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 304 (original), 312 (patched)
<https://reviews.apache.org/r/60883/#comment256540>
Why is this public?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
Line 59 (original), 59 (patched)
<https://reviews.apache.org/r/60883/#comment256538>
Why this change? Ommitting public works fine.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
Lines 19 (patched)
<https://reviews.apache.org/r/60883/#comment256542>
Why this test lives here rather then in the same package as HMSFollower?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 213 (patched)
<https://reviews.apache.org/r/60883/#comment256543>
Do you need @Test here?
Please document what this test case is testing
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 223 (patched)
<https://reviews.apache.org/r/60883/#comment256544>
Please document what this case is testing. Same for tests below.
- Alexander Kolbasov
On July 21, 2017, 2:30 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 21, 2017, 2:30 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
>
>
> Diff: https://reviews.apache.org/r/60883/diff/5/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/#review181255
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On July 21, 2017, 8:24 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> -----------------------------------------------------------
>
> (Updated July 21, 2017, 8:24 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
> 2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 62fde2c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
>
>
> Diff: https://reviews.apache.org/r/60883/diff/6/
>
>
> Testing
> -------
>
> create integration tests. reproduced the issue and verfied the fix
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
(Updated July 21, 2017, 8:24 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 62fde2c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
Diff: https://reviews.apache.org/r/60883/diff/6/
Changes: https://reviews.apache.org/r/60883/diff/5-6/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
(Updated July 21, 2017, 2:30 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java e1312bf
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 547a61f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fdf52bf
Diff: https://reviews.apache.org/r/60883/diff/5/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
(Updated July 19, 2017, 10:22 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 083ee4c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d67c162
Diff: https://reviews.apache.org/r/60883/diff/4/
Changes: https://reviews.apache.org/r/60883/diff/3-4/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
(Updated July 18, 2017, 7:06 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 083ee4c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d67c162
Diff: https://reviews.apache.org/r/60883/diff/3/
Changes: https://reviews.apache.org/r/60883/diff/2-3/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li
Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table
doesn't cleanup the permissions associated with it
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60883/
-----------------------------------------------------------
(Updated July 17, 2017, 9:04 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Repository: sentry
Description
-------
There are several problems:
1) HMSFollower should read sentry-site.xml to get the authen server name, not from hive-site.xml through hiveConf. The input configuration for HMSFollower constructor is from sentry-site.xm. We should use that configuration to get server name.
2) There are two variable names that could hold the value of the server. "hive.sentry.server"is deprecated and is in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We should check the sentry.hive.server first. If it is not set, check the deprecated hive.sentry.server to be backward compatible. If it is still not set, use default value
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4de755f
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java dccbbb6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 3d67401
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java a9d05b1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java 2095469
Diff: https://reviews.apache.org/r/60883/diff/2/
Changes: https://reviews.apache.org/r/60883/diff/1-2/
Testing
-------
create integration tests. reproduced the issue and verfied the fix
Thanks,
Na Li