sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ak...@apache.org
Subject sentry git commit: SENTRY-1556: Simplify privilege cleaning (Kalyan Kumar Kalvagadda, Reviewed by: Alex Kolbasov)
Date Fri, 24 Mar 2017 00:52:35 GMT
Repository: sentry
Updated Branches:
  refs/heads/master 4ccc45320 -> a913b9d0e


SENTRY-1556: Simplify privilege cleaning (Kalyan Kumar Kalvagadda, Reviewed by: Alex Kolbasov)


Project: http://git-wip-us.apache.org/repos/asf/sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/a913b9d0
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/a913b9d0
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/a913b9d0

Branch: refs/heads/master
Commit: a913b9d0ed66f2ebbc81712e88605a3b7fb4fa8c
Parents: 4ccc453
Author: Alexander Kolbasov <akolb@cloudera.com>
Authored: Thu Mar 23 17:52:12 2017 -0700
Committer: Alexander Kolbasov <akolb@cloudera.com>
Committed: Thu Mar 23 17:52:12 2017 -0700

----------------------------------------------------------------------
 .../sentry/service/thrift/ServiceConstants.java |   2 -
 .../service/persistent/DelegateSentryStore.java |   2 -
 .../provider/db/service/model/MSentryRole.java  |  12 +-
 .../db/service/persistent/SentryStore.java      | 388 ++++++++-----------
 .../service/persistent/TestSentryRole.java      | 172 ++++++++
 .../db/service/persistent/TestSentryStore.java  | 199 +++++++++-
 6 files changed, 515 insertions(+), 260 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
b/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
index f9fb0f3..de97a31 100644
--- a/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
+++ b/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
@@ -119,8 +119,6 @@ public class ServiceConstants {
     public static final String SENTRY_STORE_LOCAL_GROUP_MAPPING = "org.apache.sentry.provider.file.LocalGroupMappingService";
     public static final String SENTRY_STORE_GROUP_MAPPING_DEFAULT = SENTRY_STORE_HADOOP_GROUP_MAPPING;
 
-    public static final String SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL = "sentry.store.orphaned.privilege.removal";
-    public static final String SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL_DEFAULT = "false";
     public static final String SENTRY_HA_ENABLED = "sentry.ha.enabled";
     public static final boolean SENTRY_HA_ENABLED_DEFAULT = false;
     public static final String SENTRY_HA_ZK_PROPERTY_PREFIX = "sentry.ha.zookeeper.";

http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
index fc84e23..3be6a0b 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
@@ -64,8 +64,6 @@ public class DelegateSentryStore implements SentryStoreLayer {
 
   DelegateSentryStore(Configuration conf) throws Exception {
     this.privilegeOperator = new PrivilegeOperatePersistence(conf);
-    // The generic model doesn't turn on the thread that cleans hive privileges
-    conf.set(ServerConfig.SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL,"false");
     this.conf = conf;
     //delegated old sentryStore
     this.delegate = new SentryStore(conf);

http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
index f1d7a86..f9da589 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
@@ -173,14 +173,16 @@ public class MSentryRole {
     }
   }
 
-  public Set<MSentryPrivilege> removePrivileges() {
-    // copy is required since privilege.removeRole will call remotePrivilege
-    Set<MSentryPrivilege> copy = ImmutableSet.copyOf(privileges);
-    for (MSentryPrivilege privilege : copy) {
+  public void removePrivileges() {
+    // As we iterate through the loop below Method removeRole will modify the privileges
set
+    // will be updated.
+    // Copy of the <code>privileges<code> is taken at the beginning of the loop
to avoid using
+    // the actual privilege set in MSentryRole instance.
+
+    for (MSentryPrivilege privilege : ImmutableSet.copyOf(privileges)) {
       privilege.removeRole(this);
     }
     Preconditions.checkState(privileges.isEmpty(), "Privileges should be empty: " + privileges);
-    return copy;
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
index 32637d2..f0d2a6b 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
@@ -32,9 +32,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
-import java.util.concurrent.locks.Condition;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
 
 import javax.jdo.FetchGroup;
 import javax.jdo.JDODataStoreException;
@@ -120,8 +117,6 @@ public class SentryStore {
 
   private final PersistenceManagerFactory pmf;
   private Configuration conf;
-  private PrivCleaner privCleaner = null;
-  private Thread privCleanerThread = null;
   private final TransactionManager tm;
 
   public SentryStore(Configuration conf) throws Exception {
@@ -200,15 +195,6 @@ public class SentryStore {
     pmf = JDOHelper.getPersistenceManagerFactory(prop);
     tm = new TransactionManager(pmf, conf);
     verifySentryStoreSchema(checkSchemaVersion);
-
-    // Kick off the thread that cleans orphaned privileges (unless told not to)
-    privCleaner = this.new PrivCleaner();
-    if (conf.get(ServerConfig.SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL,
-            ServerConfig.SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL_DEFAULT)
-            .equalsIgnoreCase("true")) {
-      privCleanerThread = new Thread(privCleaner);
-      privCleanerThread.start();
-    }
   }
 
   public TransactionManager getTransactionManager() {
@@ -232,14 +218,6 @@ public class SentryStore {
   }
 
   public synchronized void stop() {
-    if (privCleanerThread != null) {
-      privCleaner.exit();
-      try {
-        privCleanerThread.join();
-      } catch (InterruptedException e) {
-        // Ignore...
-      }
-    }
     if (pmf != null) {
       pmf.close();
     }
@@ -487,12 +465,10 @@ public class SentryStore {
           MSentryPrivilege mInsert = getMSentryPrivilege(tNotAll, pm);
           if (mSelect != null && mRole.getPrivileges().contains(mSelect)) {
             mSelect.removeRole(mRole);
-            privCleaner.incPrivRemoval();
             pm.makePersistent(mSelect);
           }
           if (mInsert != null && mRole.getPrivileges().contains(mInsert)) {
             mInsert.removeRole(mRole);
-            privCleaner.incPrivRemoval();
             pm.makePersistent(mInsert);
           }
         } else {
@@ -584,41 +560,59 @@ public class SentryStore {
    * privilege and add SELECT (INSERT was revoked) or INSERT (SELECT was revoked).
    */
   private void revokePartial(PersistenceManager pm,
-      TSentryPrivilege requestedPrivToRevoke, MSentryRole mRole,
-      MSentryPrivilege currentPrivilege) throws SentryInvalidInputException {
-    MSentryPrivilege persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege),
pm);
+                             TSentryPrivilege requestedPrivToRevoke, MSentryRole mRole,
+                             MSentryPrivilege currentPrivilege) throws SentryInvalidInputException
{
+    MSentryPrivilege persistedPriv =
+      getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm);
     if (persistedPriv == null) {
+      // The privilege corresponding to the currentPrivilege doesn't exist in the persistent
+      // store, so we create a fake one for the code below. The fake one is not associated
with
+      // any role and shouldn't be stored in the persistent storage.
       persistedPriv = convertToMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege));
     }
 
-    if (requestedPrivToRevoke.getAction().equalsIgnoreCase("ALL") || requestedPrivToRevoke.getAction().equalsIgnoreCase("*"))
{
-      persistedPriv.removeRole(mRole);
-      privCleaner.incPrivRemoval();
-      pm.makePersistent(persistedPriv);
+    if (requestedPrivToRevoke.getAction().equalsIgnoreCase("ALL") ||
+      requestedPrivToRevoke.getAction().equalsIgnoreCase("*")) {
+      if (!persistedPriv.getRoles().isEmpty()) {
+        persistedPriv.removeRole(mRole);
+        if (persistedPriv.getRoles().isEmpty()) {
+          pm.deletePersistent(persistedPriv);
+        } else {
+          pm.makePersistent(persistedPriv);
+        }
+      }
     } else if (requestedPrivToRevoke.getAction().equalsIgnoreCase(AccessConstants.SELECT)
-        && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.INSERT))
{
+      && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.INSERT))
{
       revokeRolePartial(pm, mRole, currentPrivilege, persistedPriv, AccessConstants.INSERT);
     } else if (requestedPrivToRevoke.getAction().equalsIgnoreCase(AccessConstants.INSERT)
-        && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.SELECT))
{
+      && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.SELECT))
{
       revokeRolePartial(pm, mRole, currentPrivilege, persistedPriv, AccessConstants.SELECT);
     }
   }
 
   private void revokeRolePartial(PersistenceManager pm, MSentryRole mRole,
-      MSentryPrivilege currentPrivilege, MSentryPrivilege persistedPriv, String addAction)
-      throws SentryInvalidInputException {
+                                 MSentryPrivilege currentPrivilege,
+                                 MSentryPrivilege persistedPriv,
+                                 String addAction) throws SentryInvalidInputException {
     // If table / URI, remove ALL
-    persistedPriv.removeRole(mRole);
-    privCleaner.incPrivRemoval();
-    pm.makePersistent(persistedPriv);
-
+    persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(persistedPriv), pm);
+    if (persistedPriv != null && !persistedPriv.getRoles().isEmpty()) {
+      persistedPriv.removeRole(mRole);
+      if (persistedPriv.getRoles().isEmpty()) {
+        pm.deletePersistent(persistedPriv);
+      } else {
+        pm.makePersistent(persistedPriv);
+      }
+    }
     currentPrivilege.setAction(AccessConstants.ALL);
     persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm);
     if (persistedPriv != null && mRole.getPrivileges().contains(persistedPriv)) {
       persistedPriv.removeRole(mRole);
-      privCleaner.incPrivRemoval();
-      pm.makePersistent(persistedPriv);
-
+      if (persistedPriv.getRoles().isEmpty()) {
+        pm.deletePersistent(persistedPriv);
+      } else {
+        pm.makePersistent(persistedPriv);
+      }
       currentPrivilege.setAction(addAction);
       persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm);
       if (persistedPriv == null) {
@@ -634,7 +628,8 @@ public class SentryStore {
    * Revoke privilege from role
    */
   private void revokePrivilegeFromRole(PersistenceManager pm, TSentryPrivilege tPrivilege,
-      MSentryRole mRole, MSentryPrivilege mPrivilege) throws SentryInvalidInputException
{
+                                       MSentryRole mRole, MSentryPrivilege mPrivilege)
+    throws SentryInvalidInputException {
     if (PARTIAL_REVOKE_ACTIONS.contains(mPrivilege.getAction())) {
       // if this privilege is in {ALL,SELECT,INSERT}
       // we will do partial revoke
@@ -643,10 +638,13 @@ public class SentryStore {
       // if this privilege is not ALL, SELECT nor INSERT,
       // we will revoke it from role directly
       MSentryPrivilege persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(mPrivilege),
pm);
-      if (persistedPriv != null) {
-        mPrivilege.removeRole(mRole);
-        privCleaner.incPrivRemoval();
-        pm.makePersistent(mPrivilege);
+      if (persistedPriv != null && !persistedPriv.getRoles().isEmpty()) {
+        persistedPriv.removeRole(mRole);
+        if (persistedPriv.getRoles().isEmpty()) {
+          pm.deletePersistent(persistedPriv);
+        } else {
+          pm.makePersistent(persistedPriv);
+        }
       }
     }
   }
@@ -805,14 +803,34 @@ public class SentryStore {
     if (sentryRole == null) {
       throw noSuchRole(lRoleName);
     }
-    int numPrivs = sentryRole.getPrivileges().size();
+    removePrivileges(pm, sentryRole);
+    pm.deletePersistent(sentryRole);
+  }
+
+  /**
+   * Removes all the privileges associated with
+   * a particular role. After this dis-association if the
+   * privilege doesn't have any roles associated it will be
+   * removed from the underlying persistence layer.
+   * @param pm Instance of PersistenceManager
+   * @param sentryRole Role for which all the privileges are to be removed.
+   */
+  private void removePrivileges(PersistenceManager pm, MSentryRole sentryRole) {
+    List<MSentryPrivilege> privilegesCopy = new ArrayList<>(sentryRole.getPrivileges());
+    List<MSentryPrivilege> stalePrivileges = new ArrayList<>(0);
+
     sentryRole.removePrivileges();
     // with SENTRY-398 generic model
     sentryRole.removeGMPrivileges();
-    privCleaner.incPrivRemoval(numPrivs);
-    pm.deletePersistent(sentryRole);
+    for (MSentryPrivilege privilege : privilegesCopy) {
+      if(privilege.getRoles().isEmpty()) {
+        stalePrivileges.add(privilege);
+      }
+    }
+    if(!stalePrivileges.isEmpty()) {
+      pm.deletePersistentAll(stalePrivileges);
+    }
   }
-
   public void alterSentryRoleAddGroups(final String grantorPrincipal,
       final String roleName, final Set<TSentryGroup> groupNames) throws Exception {
     tm.executeTransactionWithRetry(
@@ -953,6 +971,53 @@ public class SentryStore {
         });
   }
 
+  /**
+   * Gets the MSentryPrivilege from sentry persistent storage based on TSentryPrivilege
+   * provided
+   *
+   * Method is currently used only in test framework
+   * @param tPrivilege
+   * @return MSentryPrivilege if the privilege is found in the storage
+   * null, if the privilege is not found in the storage.
+   * @throws Exception
+   */
+  @VisibleForTesting
+  MSentryPrivilege findMSentryPrivilegeFromTSentryPrivilege(final TSentryPrivilege tPrivilege)
throws Exception {
+    return tm.executeTransaction(
+      new TransactionBlock<MSentryPrivilege>() {
+        public MSentryPrivilege execute(PersistenceManager pm) throws Exception {
+          return getMSentryPrivilege(tPrivilege, pm);
+        }
+      });
+  }
+
+  /**
+   * Returns a list with all the privileges in the sentry persistent storage
+   *
+   * Method is currently used only in test framework
+   * @return List of all sentry privileges in the store
+   * @throws Exception
+   */
+  @VisibleForTesting
+  List<MSentryPrivilege> getAllMSentryPrivileges () throws Exception {
+    return tm.executeTransaction(
+      new TransactionBlock<List<MSentryPrivilege>>() {
+        public List<MSentryPrivilege> execute(PersistenceManager pm) throws Exception
{
+          return getAllMSentryPrivilegesCore(pm);
+        }
+      });
+  }
+
+  /**
+   * Method Returns all the privileges present in the persistent store as a list.
+   * @param pm PersistenceManager
+   * @returns list of all the privileges in the persistent store
+   */
+  private List<MSentryPrivilege> getAllMSentryPrivilegesCore (PersistenceManager pm)
{
+    Query query = pm.newQuery(MSentryPrivilege.class);
+    return (List<MSentryPrivilege>) query.execute();
+  }
+
   private boolean hasAnyServerPrivileges(final Set<String> roleNames, final String
serverName) throws Exception {
     if (roleNames == null || roleNames.isEmpty()) {
       return false;
@@ -1661,7 +1726,12 @@ public class SentryStore {
       // 1. get privilege and child privileges
       Set<MSentryPrivilege> privilegeGraph = Sets.newHashSet();
       if (parent != null) {
-        privilegeGraph.add(parent);
+
+        //  Parent privilege object is used after revoking.
+        //  If the privilege was associated to only role which is revoked,
+        //  privilege object should not be used. It is safe to insert
+        //  a copy of the parent object in the privilegeGraph
+        privilegeGraph.add(convertToMSentryPrivilege(convertToTSentryPrivilege(parent)));
         populateChildren(pm, Sets.newHashSet(role.getRoleName()), parent, privilegeGraph);
       } else {
         populateChildren(pm, Sets.newHashSet(role.getRoleName()), convertToMSentryPrivilege(tPrivilege),
@@ -1897,195 +1967,45 @@ public class SentryStore {
   }
 
   /**
-   * This thread exists to clean up "orphaned" privilege rows in the database.
-   * These rows aren't removed automatically due to the fact that there is
-   * a many-to-many mapping between the roles and privileges, and the
-   * detection and removal of orphaned privileges is a wee bit involved.
-   * This thread hangs out until notified by the parent (the outer class)
-   * and then runs a custom SQL statement that detects and removes orphans.
+   * Method detects orphaned privileges
+   *
+   * @return True, If there are orphan privileges
+   * False, If orphan privileges are not found.
+   * non-zero value if an orphan is found.
+   * <p>
+   * Method currently used only by tests.
+   * <p>
    */
-  private class PrivCleaner implements Runnable {
-    // Kick off priv orphan removal after this many notifies
-    private static final int NOTIFY_THRESHOLD = 50;
-
-    // How many times we've been notified; reset to zero after orphan removal
-    private int currentNotifies = 0;
-
-    // Internal state for threads
-    private boolean exitRequired = false;
-
-    // This lock and condition are needed to implement a way to drop the
-    // lock inside a while loop, and not hold the lock across the orphan
-    // removal.
-    private final Lock lock = new ReentrantLock();
-    private final Condition cond = lock.newCondition();
-
-    /**
-     * Waits in a loop, running the orphan removal function when notified.
-     * Will exit after exitRequired is set to true by exit().  We are careful
-     * to not hold our lock while removing orphans; that operation might
-     * take a long time.  There's also the matter of lock ordering.  Other
-     * threads start a transaction first, and then grab our lock; this thread
-     * grabs the lock and then starts a transaction.  Handling this correctly
-     * requires explicit locking/unlocking through the loop.
-     */
-    public void run() {
-      while (true) {
-        lock.lock();
-        try {
-          // Check here in case this was set during removeOrphanedPrivileges()
-          if (exitRequired) {
-            return;
-          }
-          while (currentNotifies <= NOTIFY_THRESHOLD) {
-            try {
-              cond.await();
-            } catch (InterruptedException e) {
-              // Interrupted
-            }
-            // Check here in case this was set while waiting
-            if (exitRequired) {
-              return;
-            }
-          }
-          currentNotifies = 0;
-        } finally {
-          lock.unlock();
-        }
-        try {
-          removeOrphanedPrivileges();
-        } catch (Exception e) {
-          LOGGER.warn("Privilege cleaning thread encountered an error: " +
-                  e.getMessage());
-        }
-      }
-    }
-
-    /**
-     * This is called when a privilege is removed from a role.  This may
-     * or may not mean that the privilege needs to be removed from the
-     * database; there may be more references to it from other roles.
-     * As a result, we'll lazily run the orphan cleaner every
-     * NOTIFY_THRESHOLD times this routine is called.
-     * @param numDeletions The number of potentially orphaned privileges
-     */
-    void incPrivRemoval(int numDeletions) {
-      if (privCleanerThread != null) {
-        try {
-          lock.lock();
-          currentNotifies += numDeletions;
-          if (currentNotifies > NOTIFY_THRESHOLD) {
-            cond.signal();
-          }
-        } finally {
-          lock.unlock();
+  @VisibleForTesting
+  Boolean findOrphanedPrivileges() throws Exception {
+    return tm.executeTransaction(
+      new TransactionBlock<Boolean>() {
+        public Boolean execute(PersistenceManager pm) throws Exception {
+          return findOrphanedPrivilegesCore(pm);
         }
-      }
-    }
+      });
+  }
 
-    /**
-     * Simple form of incPrivRemoval when only one privilege is deleted.
-     */
-    void incPrivRemoval() {
-      incPrivRemoval(1);
+  Boolean findOrphanedPrivilegesCore(PersistenceManager pm) {
+    ArrayList<Object> idList = new ArrayList<Object>();
+    //Perform a SQL query to get things that look like orphans
+    List<MSentryPrivilege> results = getAllMSentryPrivilegesCore(pm);
+    for (MSentryPrivilege orphan : results) {
+      idList.add(pm.getObjectId(orphan));
     }
-
-    /**
-     * Tell this thread to exit. Safe to call multiple times, as it just
-     * notifies the run() loop to finish up.
-     */
-    void exit() {
-      if (privCleanerThread != null) {
-        lock.lock();
-        try {
-          exitRequired = true;
-          cond.signal();
-        } finally {
-          lock.unlock();
-        }
-      }
+    if (idList.isEmpty()) {
+      return false;
     }
-
-    /**
-     * Run a SQL query to detect orphaned privileges, and then delete
-     * each one.  This is complicated by the fact that datanucleus does
-     * not seem to play well with the mix between a direct SQL query
-     * and operations on the database.  The solution that seems to work
-     * is to split the operation into two transactions: the first is
-     * just a read for privileges that look like they're orphans, the
-     * second transaction will go and get each of those privilege objects,
-     * verify that there are no roles attached, and then delete them.
-     */
-    @SuppressWarnings("unchecked")
-    private void removeOrphanedPrivileges() {
-      final String privDB = "SENTRY_DB_PRIVILEGE";
-      final String privId = "DB_PRIVILEGE_ID";
-      final String mapDB = "SENTRY_ROLE_DB_PRIVILEGE_MAP";
-      final String privFilter =
-              "select " + privId +
-              " from " + privDB + " p" +
-              " where not exists (" +
-                  " select 1 from " + mapDB + " d" +
-                  " where p." + privId + " != d." + privId +
-              " )";
-      boolean rollback = true;
-      int orphansRemoved = 0;
-      ArrayList<Object> idList = new ArrayList<>();
-      PersistenceManager pm = pmf.getPersistenceManager();
-
-      // Transaction 1: Perform a SQL query to get things that look like orphans
-      try {
-        Transaction transaction = pm.currentTransaction();
-        transaction.begin();
-        transaction.setRollbackOnly();  // Makes the tx read-only
-        Query query = pm.newQuery("javax.jdo.query.SQL", privFilter);
-        query.setClass(MSentryPrivilege.class);
-        List<MSentryPrivilege> results = (List<MSentryPrivilege>) query.execute();
-        for (MSentryPrivilege orphan : results) {
-          idList.add(pm.getObjectId(orphan));
-        }
-        transaction.rollback();
-        rollback = false;
-      } finally {
-        if (rollback && pm.currentTransaction().isActive()) {
-          pm.currentTransaction().rollback();
-        } else {
-          LOGGER.debug("Found {} potential orphans", idList.size());
-        }
-      }
-
-      if (idList.isEmpty()) {
-        pm.close();
-        return;
-      }
-
-      Preconditions.checkState(!rollback);
-
-      // Transaction 2: For each potential orphan, verify it's really an
-      // orphan and delete it if so
-      rollback = true;
-      try {
-        Transaction transaction = pm.currentTransaction();
-        transaction.begin();
-        pm.refreshAll();  // Try to ensure we really have correct objects
-        for (Object id : idList) {
-          MSentryPrivilege priv = (MSentryPrivilege) pm.getObjectById(id);
-          if (priv.getRoles().isEmpty()) {
-            pm.deletePersistent(priv);
-            orphansRemoved++;
-          }
-        }
-        transaction.commit();
-        pm.close();
-        rollback = false;
-      } finally {
-        if (rollback) {
-          rollbackTransaction(pm);
-        } else {
-          LOGGER.debug("Cleaned up {} orphaned privileges", orphansRemoved);
-        }
+    //For each potential orphan, verify it's really a orphan.
+    // Moment an orphan is identified return 1 indicating an orphan is found.
+    pm.refreshAll();  // Try to ensure we really have correct objects
+    for (Object id : idList) {
+      MSentryPrivilege priv = (MSentryPrivilege) pm.getObjectById(id);
+      if (priv.getRoles().isEmpty()) {
+        return true;
       }
     }
+    return false;
   }
 
   /** get mapping datas for [group,role], [user,role] with the specific roles */

http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
index 29134fe..9be4a8b 100644
--- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
+++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
@@ -24,6 +24,7 @@ import static org.junit.Assert.fail;
 import java.io.File;
 import java.util.Arrays;
 import java.util.Properties;
+import java.util.List;
 
 import javax.jdo.JDOHelper;
 import javax.jdo.PersistenceManager;
@@ -47,6 +48,8 @@ import com.google.common.io.Files;
 /**
  * The class tests that the new feature SENTRY-398 generic model adds the new field in the
MSentryRole
  * will not affect the functionality of the origin hive/impala authorization model
+ * Some Tests below make sure that privileges are removed from sentry storage the moment
they are not associated to any role.
+ * This avoid the need for PrivCleaner to perform periodic cleanup.
  */
 public class TestSentryRole {
   private static PersistenceManagerFactory pmf;
@@ -342,6 +345,175 @@ public class TestSentryRole {
     commitTransaction(pm);
   }
 
+  /**
+   * Removes a role and makes sure that privileges are removed from sentry storage
+   * moment they are not associated to any role.
+   * @throws Exception
+   */
+  @Test
+  public void testDeleteRole() throws Exception {
+    String roleName = "r1";
+    //hive/impala privilege
+    MSentryPrivilege hivePrivilege = new MSentryPrivilege();
+    hivePrivilege.setServerName("hive.server1");
+    hivePrivilege.setDbName("db1");
+    hivePrivilege.setTableName("tb1");
+    hivePrivilege.setPrivilegeScope("table");
+    hivePrivilege.setAction("select");
+    hivePrivilege.setURI(SentryStore.NULL_COL);
+    hivePrivilege.setColumnName(SentryStore.NULL_COL);
+    hivePrivilege.setGrantOption(true);
+
+    //solr privilege
+    MSentryGMPrivilege solrPrivilege = new MSentryGMPrivilege();
+    solrPrivilege.setComponentName("solr");
+    solrPrivilege.setServiceName("solr.server1");
+    solrPrivilege.setAuthorizables(Arrays.asList(new Collection("c1")));
+    solrPrivilege.setAction("query");
+    solrPrivilege.setGrantOption(true);
+
+    PersistenceManager pm = null;
+    //create role
+    pm = openTransaction();
+    pm.makePersistent(new MSentryRole(roleName, System.currentTimeMillis()));
+    commitTransaction(pm);
+
+    //grant hivePrivilege and solrPrivilege to role
+    pm = openTransaction();
+    MSentryRole role = getMSentryRole(pm, roleName);
+    hivePrivilege.appendRole(role);
+    solrPrivilege.appendRole(role);
+    pm.makePersistent(hivePrivilege);
+    pm.makePersistent(solrPrivilege);
+    pm.makePersistent(role);
+    commitTransaction(pm);
+
+    //check
+    pm = openTransaction();
+    role = getMSentryRole(pm, roleName);
+    pm.retrieve(role);
+    assertEquals(1, role.getPrivileges().size());
+    assertEquals(1, role.getGmPrivileges().size());
+    commitTransaction(pm);
+
+    //delete role
+    pm = openTransaction();
+    role = getMSentryRole(pm, roleName);
+
+    //  pm.deletePersistent(role);
+    role.removePrivileges();
+    role.removeGMPrivileges();
+    pm.deletePersistent(role);
+    commitTransaction(pm);
+
+    //check for privileges
+    //There shouldn't be any privilages
+    pm = openTransaction();
+    Query query = pm.newQuery(MSentryPrivilege.class);
+    List<MSentryPrivilege> results = (List<MSentryPrivilege>) query.execute();
+    assertEquals(1, results.size());
+    Query query1 = pm.newQuery(MSentryGMPrivilege.class);
+    List<MSentryGMPrivilege> results1 = (List<MSentryGMPrivilege>) query1.execute();
+    assertEquals(1, results1.size());
+    commitTransaction(pm);
+
+    //check
+    pm = openTransaction();
+    role = getMSentryRole(pm, roleName);
+    assertTrue(role == null);
+    commitTransaction(pm);
+  }
+
+  /**
+   * Removes a role and makes sure that privileges are not removed from sentry storage if
+   * they are associated to any other role as well.
+   * @throws Exception
+   */
+  @Test
+  public void testDeleteRole1() throws Exception {
+    String roleName1 = "r1";
+    String roleName2 = "r2";
+    //hive/impala privilege
+    MSentryPrivilege hivePrivilege = new MSentryPrivilege();
+    hivePrivilege.setServerName("hive.server1");
+    hivePrivilege.setDbName("db1");
+    hivePrivilege.setTableName("tb1");
+    hivePrivilege.setPrivilegeScope("table");
+    hivePrivilege.setAction("select");
+    hivePrivilege.setURI(SentryStore.NULL_COL);
+    hivePrivilege.setColumnName(SentryStore.NULL_COL);
+    hivePrivilege.setGrantOption(true);
+
+    //solr privilege
+    MSentryGMPrivilege solrPrivilege = new MSentryGMPrivilege();
+    solrPrivilege.setComponentName("solr");
+    solrPrivilege.setServiceName("solr.server1");
+    solrPrivilege.setAuthorizables(Arrays.asList(new Collection("c1")));
+    solrPrivilege.setAction("query");
+    solrPrivilege.setGrantOption(true);
+
+    PersistenceManager pm = null;
+    //create role1
+    pm = openTransaction();
+    pm.makePersistent(new MSentryRole(roleName1, System.currentTimeMillis()));
+    commitTransaction(pm);
+
+    //create role2
+    pm = openTransaction();
+    pm.makePersistent(new MSentryRole(roleName2, System.currentTimeMillis()));
+    commitTransaction(pm);
+
+    //grant hivePrivilege and solrPrivilege to role1 and role2
+    pm = openTransaction();
+    MSentryRole role1 = getMSentryRole(pm, roleName1);
+    MSentryRole role2 = getMSentryRole(pm, roleName2);
+    hivePrivilege.appendRole(role1);
+    solrPrivilege.appendRole(role1);
+    hivePrivilege.appendRole(role2);
+    solrPrivilege.appendRole(role2);
+    pm.makePersistent(hivePrivilege);
+    pm.makePersistent(solrPrivilege);
+    pm.makePersistent(role1);
+    pm.makePersistent(role2);
+    commitTransaction(pm);
+
+    //check
+    pm = openTransaction();
+    role1 = getMSentryRole(pm, roleName1);
+    pm.retrieve(role1);
+    assertEquals(1, role1.getPrivileges().size());
+    assertEquals(1, role1.getGmPrivileges().size());
+    role2 = getMSentryRole(pm, roleName2);
+    pm.retrieve(role2);
+    assertEquals(1, role2.getPrivileges().size());
+    assertEquals(1, role2.getGmPrivileges().size());
+    commitTransaction(pm);
+
+    //delete role
+    pm = openTransaction();
+    role1 = getMSentryRole(pm, roleName1);
+    role1.removePrivileges();
+    role1.removeGMPrivileges();
+    pm.deletePersistent(role1);
+    commitTransaction(pm);
+
+    //check for privileges
+    //Privileges should be present
+    pm = openTransaction();
+    Query query = pm.newQuery(MSentryPrivilege.class);
+    List<MSentryPrivilege> results = (List<MSentryPrivilege>) query.execute();
+    assertEquals(1, results.size());
+    Query query1 = pm.newQuery(MSentryGMPrivilege.class);
+    List<MSentryGMPrivilege> results1 = (List<MSentryGMPrivilege>) query1.execute();
+    assertEquals(1, results1.size());
+    commitTransaction(pm);
+
+    //check
+    pm = openTransaction();
+    role1 = getMSentryRole(pm, roleName1);
+    assertTrue(role1 == null);
+    commitTransaction(pm);
+  }
   private PersistenceManager openTransaction() {
     PersistenceManager pm = pmf.getPersistenceManager();
     Transaction currentTransaction = pm.currentTransaction();

http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
index 20ce392..3327e68 100644
--- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
+++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
@@ -24,6 +24,7 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.List;
 
 import org.apache.commons.io.FileUtils;
 import org.apache.hadoop.conf.Configuration;
@@ -49,7 +50,6 @@ import org.junit.After;
 import org.junit.AfterClass;
 import org.junit.Before;
 import org.junit.BeforeClass;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import com.google.common.collect.Iterables;
@@ -66,7 +66,7 @@ public class TestSentryStore extends org.junit.Assert {
   private static String[] adminGroups = { "adminGroup1" };
   private static PolicyFile policyFile;
   private static File policyFilePath;
-  final long NUM_PRIVS = 60;  // > SentryStore.PrivCleaner.NOTIFY_THRESHOLD
+  final long NUM_PRIVS = 5;  // > SentryStore.PrivCleaner.NOTIFY_THRESHOLD
   private static Configuration conf = null;
   private static char[] passwd = new char[] { '1', '2', '3'};
 
@@ -509,21 +509,12 @@ public class TestSentryStore extends org.junit.Assert {
     assertEquals(table, mPrivilege.getTableName());
     assertEquals(AccessConstants.INSERT, mPrivilege.getAction());
     assertFalse(mPrivilege.getGrantOption());
+    long numDBPrivs = sentryStore.countMSentryPrivileges();
+    assertEquals("Privilege count", numDBPrivs,1);
   }
 
   private void verifyOrphanCleanup() throws Exception {
-    boolean success = false;
-    int iterations = 30;
-    while (!success && iterations > 0) {
-      Thread.sleep(1000);
-      long numDBPrivs = sentryStore.countMSentryPrivileges();
-      if (numDBPrivs < NUM_PRIVS) {
-        assertEquals(0, numDBPrivs);
-        success = true;
-      }
-      iterations--;
-    }
-    assertTrue("Failed to cleanup orphaned privileges", success);
+    assertFalse("Failed to cleanup orphaned privileges", sentryStore.findOrphanedPrivileges());
   }
 
   /**
@@ -532,7 +523,7 @@ public class TestSentryStore extends org.junit.Assert {
    * cleanup thread runs, and expect them all to be gone from the database.
    * @throws Exception
    */
-  @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems")
+ // @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems")
   @Test
   public void testPrivilegeCleanup() throws Exception {
     final String roleName = "test-priv-cleanup";
@@ -556,7 +547,7 @@ public class TestSentryStore extends org.junit.Assert {
     }
 
     // Make sure we really have the expected number of privs in the database
-    assertEquals(sentryStore.countMSentryPrivileges(), NUM_PRIVS);
+     assertEquals(sentryStore.countMSentryPrivileges(), NUM_PRIVS);
 
     // Now to make a bunch of orphans, we just remove the role that
     // created them.
@@ -564,6 +555,9 @@ public class TestSentryStore extends org.junit.Assert {
 
     // Now wait and see if the orphans get cleaned up
     verifyOrphanCleanup();
+
+    List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges();
+    assertEquals(list.size(), 0);
   }
 
   /**
@@ -574,7 +568,7 @@ public class TestSentryStore extends org.junit.Assert {
    * cleanup thread.
    * @throws Exception
    */
-  @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems")
+ // @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems")
   @Test
   public void testPrivilegeCleanup2() throws Exception {
     final String roleName = "test-priv-cleanup";
@@ -612,8 +606,179 @@ public class TestSentryStore extends org.junit.Assert {
     // Drop the role and clean up as before
     sentryStore.dropSentryRole(roleName);
     verifyOrphanCleanup();
+    //There should not be any Privileges left
+    List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges();
+    assertEquals(list.size(), 0);
+  }
+
+  /**
+   * This method tries to add ALL privileges on
+   * databases to a role and immediately revokes
+   * SELECT and INSERT privileges. At the end of
+   * each iteration we should not find any privileges
+   * for that role. Finally we should not find any
+   * privileges, as we are cleaning up orphan privileges
+   * immediately.
+   * @throws Exception
+   */
+  @Test
+  public void testPrivilegeCleanup3() throws Exception {
+    final String roleName = "test-priv-cleanup";
+    final String grantor = "g1";
+    final String server = "server";
+    final String dBase = "db";
+    final String table = "table-";
+
+    createRole(roleName);
+
+    // Create NUM_PRIVS unique privilege objects in the database once more,
+    // this time granting ALL and revoking SELECT to make INSERT.
+    for (int i=0 ; i < NUM_PRIVS; i++) {
+      TSentryPrivilege priv = new TSentryPrivilege();
+      priv.setPrivilegeScope("DATABASE");
+      priv.setServerName(server);
+      priv.setAction(AccessConstants.ALL);
+      priv.setCreateTime(System.currentTimeMillis());
+      priv.setTableName(table + i);
+      priv.setDbName(dBase);
+      priv.setGrantOption(TSentryGrantOption.TRUE);
+      sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, priv);
+
+      priv.setAction(AccessConstants.SELECT);
+      priv.setGrantOption(TSentryGrantOption.UNSET);
+      sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv);
+
+      sentryStore.findOrphanedPrivileges();
+      //After having ALL and revoking SELECT, we should have INSERT
+      //Remove the INSERT privilege as well.
+      //There should not be any more privileges in the sentry store
+      priv.setAction(AccessConstants.INSERT);
+      sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv);
+
+      MSentryRole role = sentryStore.getMSentryRoleByName(roleName);
+      assertEquals("Privilege Count", 0, role.getPrivileges().size());
+
+    }
+
+    // Drop the role and clean up as before
+    verifyOrphanCleanup();
+
+    //There should not be any Privileges left
+    List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges();
+    assertEquals(list.size(), 0);
+
   }
 
+  /**
+   * This method "n" privileges with action "ALL" on
+   * tables to a role. Later we are revoking insert
+   * and select privileges to of the tables making the
+   * the privilege orphan. Finally we should find only
+   * n -1 privileges, as we are cleaning up orphan
+   * privileges immediately.
+   * @throws Exception
+   */
+  @Test
+  public void testPrivilegeCleanup4 () throws Exception {
+    final String roleName = "test-priv-cleanup";
+    final String grantor = "g1";
+    final String server = "server";
+    final String dBase = "db";
+    final String table = "table-";
+
+    createRole(roleName);
+
+    // Create NUM_PRIVS unique privilege objects in the database
+    for (int i = 0; i < NUM_PRIVS; i++) {
+      TSentryPrivilege priv = new TSentryPrivilege();
+      priv.setPrivilegeScope("TABLE");
+      priv.setServerName(server);
+      priv.setAction(AccessConstants.ALL);
+      priv.setCreateTime(System.currentTimeMillis());
+      priv.setTableName(table + i);
+      priv.setDbName(dBase);
+      sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, priv);
+    }
+
+    // Make sure we really have the expected number of privs in the database
+    assertEquals(sentryStore.countMSentryPrivileges(), NUM_PRIVS);
+
+    //Revoking INSERT privilege. This is change the privilege to SELECT
+    TSentryPrivilege priv = new TSentryPrivilege();
+    priv.setPrivilegeScope("TABLE");
+    priv.setServerName(server);
+    priv.setAction(AccessConstants.INSERT);
+    priv.setCreateTime(System.currentTimeMillis());
+    priv.setTableName(table + '0');
+    priv.setDbName(dBase);
+    sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv);
+
+    //There should be SELECT privilege in the sentry store
+    priv = new TSentryPrivilege();
+    priv.setPrivilegeScope("TABLE");
+    priv.setServerName(server);
+    priv.setAction(AccessConstants.SELECT);
+    priv.setCreateTime(System.currentTimeMillis());
+    priv.setTableName(table + '0');
+    priv.setDbName(dBase);
+    MSentryPrivilege mPriv = sentryStore.findMSentryPrivilegeFromTSentryPrivilege(priv);
+    assertNotNull(mPriv);
+
+    MSentryRole role = sentryStore.getMSentryRoleByName(roleName);
+    assertEquals("Privilege Count", NUM_PRIVS, role.getPrivileges().size());
+
+    sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv);
+    role = sentryStore.getMSentryRoleByName(roleName);
+    assertEquals("Privilege Count", NUM_PRIVS-1, role.getPrivileges().size());
+
+  }
+
+  /**
+   * This method tries to add alter privileges on
+   * databases to a role and immediately revokes
+   * them. At the end of each iteration we should
+   * not find any privileges for that role
+   * Finally we should not find any privileges, as
+   * we are cleaning up orphan privileges immediately.
+   * @throws Exception
+   */
+  @Test
+  public void testPrivilegeCleanup5() throws Exception {
+    final String roleName = "test-priv-cleanup";
+    final String grantor = "g1";
+    final String server = "server";
+    final String dBase = "db";
+    final String table = "table-";
+
+    createRole(roleName);
+
+    // Create NUM_PRIVS unique privilege objects in the database once more,
+    // this time granting ALL and revoking SELECT to make INSERT.
+    for (int i=0 ; i < NUM_PRIVS; i++) {
+      TSentryPrivilege priv = new TSentryPrivilege();
+      priv.setPrivilegeScope("DATABASE");
+      priv.setServerName(server);
+      priv.setAction(AccessConstants.ALTER);
+      priv.setCreateTime(System.currentTimeMillis());
+      priv.setTableName(table + i);
+      priv.setDbName(dBase);
+      priv.setGrantOption(TSentryGrantOption.TRUE);
+      sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, priv);
+
+      priv.setAction(AccessConstants.ALTER);
+      sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv);
+
+      MSentryRole role = sentryStore.getMSentryRoleByName(roleName);
+      assertEquals("Privilege Count", 0, role.getPrivileges().size());
+    }
+    //There should not be any Privileges left
+    List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges();
+    assertEquals(list.size(), 0);
+
+  }
+
+  //TODO Use new transaction Manager logic, Instead of
+
   @Test
   public void testGrantRevokeMultiPrivileges() throws Exception {
     String roleName = "test-privilege";


Mime
View raw message