You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arun Suresh <ar...@gmail.com> on 2014/12/15 10:28:58 UTC
Review Request 29041: SENTRY-567: SentryStore Decorator that adds
Locking
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29041/
-----------------------------------------------------------
Review request for sentry, bc Wong, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
This patch includes a base SentryStore Decorator (LockingSentryStore) that adds locking to an existing SentryStore
It also inclues two implementations for the Decorator
1) SentryStoreWithLocalLock uses the standard java.util.concurrent.ReadWriteLock.. usefull for single instance deployments of the Sentry Service
2) SentryStoreWithDistributedLock implements a Distributed Lock. It uses the Hazelcast (http://hazelcast.org) library.
More decription on the JIRA : issues.apache.org/jira/browse/SENTRY-567
TODO: Another possible implementation using Zookeeper. HazelCast and Zookeeper has its own merits. Zookeeper might be better for global lock consistency since it denies availability to peers that are not part of the Quorum in the case of partition tolerence. Hazelcast chooses availibliity but APIs are simpler than Zookeeper.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java PRE-CREATION
Diff: https://reviews.apache.org/r/29041/diff/
Testing
-------
Thanks,
Arun Suresh
Re: Review Request 29041: SENTRY-567: SentryStore Decorator that adds
Locking
Posted by Xiaomeng Huang <xi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29041/#review68577
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
<https://reviews.apache.org/r/29041/#comment112901>
I know you want to get HAContext via "synchronized static HAContext get()" instead of creating from constructor. But only remove public from constructor, we also can create HAContext via constructor in same package. We should change public to private.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java
<https://reviews.apache.org/r/29041/#comment112902>
please put the license to the top.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java
<https://reviews.apache.org/r/29041/#comment112900>
I think LockingSentryStore is used to decorate InMemSentryStore. So we'd better extends InMemSentryStore rather than implements SentryStore.
This patch looks fine to me. A few comments, please feel free to fix them.
- Xiaomeng Huang
On 十二月 18, 2014, 8:04 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29041/
> -----------------------------------------------------------
>
> (Updated 十二月 18, 2014, 8:04 a.m.)
>
>
> Review request for sentry, bc Wong, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch includes a base SentryStore Decorator (LockingSentryStore) that adds locking to an existing SentryStore
> It also inclues two implementations for the Decorator
> 1) SentryStoreWithLocalLock uses the standard java.util.concurrent.ReadWriteLock.. usefull for single instance deployments of the Sentry Service
> 2) SentryStoreWithDistributedLock implements a Distributed Lock. It uses the Hazelcast (http://hazelcast.org) library.
> More decription on the JIRA : issues.apache.org/jira/browse/SENTRY-567
>
> TODO: Another possible implementation using Zookeeper. HazelCast and Zookeeper has its own merits. Zookeeper might be better for global lock consistency since it denies availability to peers that are not part of the Quorum in the case of partition tolerence. Hazelcast chooses availibliity but APIs are simpler than Zookeeper.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 523261e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29041/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 29041: SENTRY-567: SentryStore Decorator that adds
Locking
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29041/#review68403
-----------------------------------------------------------
Hi Arun, the feature is great! Some comments below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
<https://reviews.apache.org/r/29041/#comment112597>
I wonder HAContext might can't be static.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java
<https://reviews.apache.org/r/29041/#comment112602>
Nit: please remove the blank.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java
<https://reviews.apache.org/r/29041/#comment112621>
It looks like writelock and readlock have many duplicate code with same logic. Can we refactor these part?
A suggestion may like:
Adding **WriteOperations** and **ReadOperations** interfaces and adding private method **runWithWriteLock(WriteOperations operation)** and **runWithReadLock(ReadOperations operation)** for **LockingSentryStore**
The createSentryRole may like these after refactor:
```java
@Override
public CommitContext createSentryRole(String roleName)
throws SentryAlreadyExistsException {
runWithWriteLock(new WriteOperations<CommitContext>(){
@Override
public CommitContext run() {
return sentryStore.createSentryRole(roleName)
}
});
}
```
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java
<https://reviews.apache.org/r/29041/#comment112601>
Nit: I wonder if we need to lock all the api for every operation. We might need to optimize it by finding the right granularity for lock in future.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java
<https://reviews.apache.org/r/29041/#comment112610>
Nit: Do you think it's better to use **SENTRY_DISTRIBUTED_LOCK_TIMEOUT_MS_DEFAULT**
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java
<https://reviews.apache.org/r/29041/#comment112611>
Nit: **SENTRY_DISTRIBUTED_LOCK_PATH_DEFAULT**
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java
<https://reviews.apache.org/r/29041/#comment112626>
It seems the **Context** is unnecessary, we have only two locks here, why not create two method **readUnlock()** and **writeUnlock()**?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java
<https://reviews.apache.org/r/29041/#comment112624>
As above: It seems the **Context** is unnecessary, we have only two locks here, why not create two method **readUnlock()** and **writeUnlock()**?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java
<https://reviews.apache.org/r/29041/#comment112622>
It seems just return the lock here, it might should be **rwLock.writeLock().lock()**
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java
<https://reviews.apache.org/r/29041/#comment112623>
It seems just return the lock here, it might should be **rwLock.readLock().lock()**
- Dapeng Sun
On 十二月 18, 2014, 4:04 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29041/
> -----------------------------------------------------------
>
> (Updated 十二月 18, 2014, 4:04 p.m.)
>
>
> Review request for sentry, bc Wong, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch includes a base SentryStore Decorator (LockingSentryStore) that adds locking to an existing SentryStore
> It also inclues two implementations for the Decorator
> 1) SentryStoreWithLocalLock uses the standard java.util.concurrent.ReadWriteLock.. usefull for single instance deployments of the Sentry Service
> 2) SentryStoreWithDistributedLock implements a Distributed Lock. It uses the Hazelcast (http://hazelcast.org) library.
> More decription on the JIRA : issues.apache.org/jira/browse/SENTRY-567
>
> TODO: Another possible implementation using Zookeeper. HazelCast and Zookeeper has its own merits. Zookeeper might be better for global lock consistency since it denies availability to peers that are not part of the Quorum in the case of partition tolerence. Hazelcast chooses availibliity but APIs are simpler than Zookeeper.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 523261e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29041/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 29041: SENTRY-567: SentryStore Decorator that adds
Locking
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29041/
-----------------------------------------------------------
(Updated Dec. 18, 2014, 8:04 a.m.)
Review request for sentry, bc Wong, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
* Updated documentation
* Changed the SentryStoreWithDistributedLock to use Zookeeper Curater rather than HazelCast for more consistent locking
Repository: sentry
Description
-------
This patch includes a base SentryStore Decorator (LockingSentryStore) that adds locking to an existing SentryStore
It also inclues two implementations for the Decorator
1) SentryStoreWithLocalLock uses the standard java.util.concurrent.ReadWriteLock.. usefull for single instance deployments of the Sentry Service
2) SentryStoreWithDistributedLock implements a Distributed Lock. It uses the Hazelcast (http://hazelcast.org) library.
More decription on the JIRA : issues.apache.org/jira/browse/SENTRY-567
TODO: Another possible implementation using Zookeeper. HazelCast and Zookeeper has its own merits. Zookeeper might be better for global lock consistency since it denies availability to peers that are not part of the Quorum in the case of partition tolerence. Hazelcast chooses availibliity but APIs are simpler than Zookeeper.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 523261e
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LockingSentryStore.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithDistributedLock.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreWithLocalLock.java PRE-CREATION
Diff: https://reviews.apache.org/r/29041/diff/
Testing
-------
Thanks,
Arun Suresh