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