> On Aug. 1, 2017, 6:40 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 170 (patched)
> > <https://reviews.apache.org/r/61047/diff/4/?file=1785324#file1785324line170>
> >
> > I feel explicitly setting value is not correct. When sentry store is
> > constructed, configuration is provided to it and it has all the information
> > to decide for it self.
> >
> > Whether the updates are to be persisted or not is an implementation
> > detail of sentry store. I don't see any need to ex[plicity expose an API to
> > set this value.
Please see Sasha's comments below. The goal is so calls who does not care about
hdfs sync needs not care about providing this info.
Option 1) Have two constructors for SentryStore. One constructor just
accepts conf and always disables saving permission deltas. Another constructor
has a parameter that tells whether it should be enabled or not. This cnstructor
should be only used for Policy (Hive) permissions.
Option 2) Do not change constructor, but add a setter in SentryStore which
sets the boolean value asking it to persist permission deltas. This setter
should only be called by policy store engine. This is probably best.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61047/#review181906
-----------------------------------------------------------
On Aug. 1, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61047/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda,
> Sergio Pena, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently, Sentry does not start HMSFollower when HDFS sync is disabled. This
> breaks PERM sync. For example, when a table/database is dropped, its
> permission should be removed when perm sync is enabled. Without HMSFollower,
> Sentry does not know when table/database is dropped, and therefore, cannot
> remove the permission accordingly.
> Sentry needs to make updates to have the follow behavior
> 1) When HDFS sync is disabled + PERM sync is disabled, do not start
> HMSFollower
> 2) when HDFS sync is enabled or PERM sync is enabled, start HMSFollower
> (Sentry change).
> 3) when HDFS sync is enabled, we save path change and perm change. nothing
> more.
> 4) when perm sync is enabled, we update perm when table/database is created,
> dropped or altered, nothing more.
>
>
> Diffs
> -----
>
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> 4cb46ab
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> f4842a9
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java
> 8a8bbd3
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> 9e8e0e7
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> f631869
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> 10d55dc
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
> 5826766
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
> 82f600b
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> a8ebf7c
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
> 1c3a4f2
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java
> a8e8a03
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
> 9b31b3c
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java
> c6c9448
>
>
> Diff: https://reviews.apache.org/r/61047/diff/5/
>
>
> Testing
> -------
>
> unit tests
>
>
> Thanks,
>
> Na Li
>
>
