Re: Review Request 29041: SENTRY-567: SentryStore Decorator that adds Locking

[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
> 
>