You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lei Xu <le...@cloudera.com> on 2017/02/22 21:20:41 UTC
Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/
-----------------------------------------------------------
Review request for sentry.
Bugs: SENTRY-1637
https://issues.apache.org/jira/browse/SENTRY-1637
Repository: sentry
Description
-------
* Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
* Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
Diffs
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 2ae9f3bda9f495b29dde0bd34c814a4cd31ae066
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java d1999b25b5f2c3e82d206e6f97cb195d0d527bab
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d9fce6939a86870e6d4ac308e3fb281534c871de
Diff: https://reviews.apache.org/r/56952/diff/
Testing
-------
mvn test -Dtest=TestSentryStore
Thanks,
Lei Xu
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/#review167177
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On Feb. 28, 2017, 7:35 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56952/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 7:35 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1637
> https://issues.apache.org/jira/browse/SENTRY-1637
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
> * Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
>
> Diff: https://reviews.apache.org/r/56952/diff/
>
>
> Testing
> -------
>
> mvn test -Dtest=TestSentryStore
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
Posted by Lei Xu <le...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 11:35 a.m.)
Review request for sentry.
Changes
-------
Add comments to required fields, to address reviews from Sasha.
Bugs: SENTRY-1637
https://issues.apache.org/jira/browse/SENTRY-1637
Repository: sentry
Description
-------
* Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
* Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
Diff: https://reviews.apache.org/r/56952/diff/
Testing
-------
mvn test -Dtest=TestSentryStore
Thanks,
Lei Xu
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/#review167014
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 98)
<https://reviews.apache.org/r/56952/#comment239118>
Please add comment for these two fields.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 190)
<https://reviews.apache.org/r/56952/#comment239119>
Please add comment explaining what's going on here.
- Alexander Kolbasov
On Feb. 24, 2017, 10:47 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56952/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 10:47 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1637
> https://issues.apache.org/jira/browse/SENTRY-1637
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
> * Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
>
> Diff: https://reviews.apache.org/r/56952/diff/
>
>
> Testing
> -------
>
> mvn test -Dtest=TestSentryStore
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
Posted by Lei Xu <le...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/
-----------------------------------------------------------
(Updated Feb. 24, 2017, 2:47 p.m.)
Review request for sentry.
Changes
-------
Update the patch to move SentryStore to `SentryService`
Bugs: SENTRY-1637
https://issues.apache.org/jira/browse/SENTRY-1637
Repository: sentry
Description
-------
* Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
* Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
Diff: https://reviews.apache.org/r/56952/diff/
Testing
-------
mvn test -Dtest=TestSentryStore
Thanks,
Lei Xu
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
Posted by Lei Xu <le...@cloudera.com>.
> On Feb. 23, 2017, 4:43 p.m., Alexander Kolbasov wrote:
> > I would suggest a simple approach:
> >
> > 1) Remove all knowledge about the priv cleaner from SentryStore itself.
> > 2) Priv cleaner needs SentryStore for DB operations, so we should create one in SentryService - it will be shared between the cleaner and HMSFollower and later we can remove one from the service register() method as well.
> > 3) Start the periodic process in SentryService itself. That's where we start other things like HMSFollower.
> >
> > I think this will make things quite simple and minimize the number of files that are touched by this change.
I updated the patch to move `SentryStore` creation to `SentryService`. However, the major changes, in terms of the number of files, are the `ProcessorFactory#register()`, which is required to pass the sentry store to the thrift processor.
I also changed HMSFollower to use the sentry store passed in.
However, it makes testing more difficult. I feel that the state of the two delta tables should be private states of `SentryStore`.
The code itself is very simple now. Do you think a test is needed?
- Lei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/#review166622
-----------------------------------------------------------
On Feb. 24, 2017, 2:47 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56952/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 2:47 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1637
> https://issues.apache.org/jira/browse/SENTRY-1637
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
> * Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
>
> Diff: https://reviews.apache.org/r/56952/diff/
>
>
> Testing
> -------
>
> mvn test -Dtest=TestSentryStore
>
>
> Thanks,
>
> Lei Xu
>
>
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change
tables.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56952/#review166622
-----------------------------------------------------------
I would suggest a simple approach:
1) Remove all knowledge about the priv cleaner from SentryStore itself.
2) Priv cleaner needs SentryStore for DB operations, so we should create one in SentryService - it will be shared between the cleaner and HMSFollower and later we can remove one from the service register() method as well.
3) Start the periodic process in SentryService itself. That's where we start other things like HMSFollower.
I think this will make things quite simple and minimize the number of files that are touched by this change.
- Alexander Kolbasov
On Feb. 22, 2017, 9:20 p.m., Lei Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56952/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2017, 9:20 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-1637
> https://issues.apache.org/jira/browse/SENTRY-1637
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread.
> * Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()```
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 2ae9f3bda9f495b29dde0bd34c814a4cd31ae066
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java d1999b25b5f2c3e82d206e6f97cb195d0d527bab
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java d9fce6939a86870e6d4ac308e3fb281534c871de
>
> Diff: https://reviews.apache.org/r/56952/diff/
>
>
> Testing
> -------
>
> mvn test -Dtest=TestSentryStore
>
>
> Thanks,
>
> Lei Xu
>
>