Repository: sentry
Updated Branches:
  refs/heads/master fcce56670 -> d9ab452f0


SENTRY-1517: SentryStore should actually use function getMSentryRole to get 
roles (Alexander Kolbasov, Reviewed by: Hao Hao)

Change-Id: I7a2973d69ea8afd8ecbd67ff88e638538016fbd2


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

Branch: refs/heads/master
Commit: d9ab452f0ba86177e741ccf147a16da551d01da7
Parents: fcce566
Author: hahao <hao....@cloudera.com>
Authored: Thu Dec 1 13:31:47 2016 -0800
Committer: hahao <hao....@cloudera.com>
Committed: Thu Dec 1 13:31:47 2016 -0800

----------------------------------------------------------------------
 .../service/persistent/DelegateSentryStore.java |  23 +-
 .../provider/db/service/model/MSentryRole.java  |   4 +
 .../db/service/persistent/SentryStore.java      | 481 ++++++++++---------
 .../persistent/TestDelegateSentryStore.java     |   4 +-
 .../db/service/persistent/TestSentryStore.java  |  80 ++-
 5 files changed, 336 insertions(+), 256 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/d9ab452f/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 12245ec..2ee06f9 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
@@ -75,7 +75,7 @@ public class DelegateSentryStore implements SentryStoreLayer {
   }
 
   private MSentryRole getRole(String roleName, PersistenceManager pm) {
-    return delegate.getMSentryRole(pm, roleName);
+    return delegate.getRole(pm, roleName);
   }
 
   @Override
@@ -92,26 +92,7 @@ public class DelegateSentryStore implements SentryStoreLayer 
{
   @Override
   public Object dropRole(final String component, final String role, final 
String requestor)
       throws Exception {
-    delegate.getTransactionManager().executeTransactionWithRetry(
-        new TransactionBlock() {
-          public Object execute(PersistenceManager pm) throws Exception {
-            String trimmedRole = toTrimmedLower(role);
-            Query query = pm.newQuery(MSentryRole.class);
-            query.setFilter("this.roleName == t");
-            query.declareParameters("java.lang.String t");
-            query.setUnique(true);
-            MSentryRole sentryRole = (MSentryRole) query.execute(trimmedRole);
-            if (sentryRole == null) {
-              throw new SentryNoSuchObjectException("Role: " + trimmedRole + " 
doesn't exist");
-            } else {
-              pm.retrieve(sentryRole);
-              sentryRole.removeGMPrivileges();
-              sentryRole.removePrivileges();
-              pm.deletePersistent(sentryRole);
-            }
-            return null;
-          }
-        });
+    delegate.dropSentryRole(toTrimmedLower(role));
     return null;
   }
 

http://git-wip-us.apache.org/repos/asf/sentry/blob/d9ab452f/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 0484eaa..6dc6918 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
@@ -54,6 +54,10 @@ public class MSentryRole {
     users = new HashSet<MSentryUser>();
   }
 
+  public MSentryRole(String roleName) {
+    this(roleName, System.currentTimeMillis());
+  }
+
   public long getCreateTime() {
     return createTime;
   }

http://git-wip-us.apache.org/repos/asf/sentry/blob/d9ab452f/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 742798d..f773a44 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
@@ -30,7 +30,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
-import java.util.UUID;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -45,16 +44,10 @@ import javax.jdo.Transaction;
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.hadoop.conf.Configuration;
-import org.apache.sentry.core.common.exception.SentryUserException;
+import org.apache.sentry.core.common.exception.*;
 import org.apache.sentry.core.common.utils.SentryConstants;
-import 
org.apache.sentry.core.common.exception.SentrySiteConfigurationException;
 import org.apache.sentry.core.model.db.AccessConstants;
 import org.apache.sentry.core.model.db.DBModelAuthorizable.AuthorizableType;
-import org.apache.sentry.core.common.exception.SentryAccessDeniedException;
-import org.apache.sentry.core.common.exception.SentryAlreadyExistsException;
-import org.apache.sentry.core.common.exception.SentryGrantDeniedException;
-import org.apache.sentry.core.common.exception.SentryInvalidInputException;
-import org.apache.sentry.core.common.exception.SentryNoSuchObjectException;
 import org.apache.sentry.provider.db.service.model.MSentryGroup;
 import org.apache.sentry.provider.db.service.model.MSentryPrivilege;
 import org.apache.sentry.provider.db.service.model.MSentryUser;
@@ -99,7 +92,9 @@ public class SentryStore {
   public static final String NULL_COL = "__NULL__";
   public static int INDEX_GROUP_ROLES_MAP = 0;
   public static int INDEX_USER_ROLES_MAP = 1;
-  static final String DEFAULT_DATA_DIR = "sentry_policy_db";
+
+  // For counters, representation of the "unknown value"
+  private static final long COUNT_VALUE_UNKNOWN = -1;
 
   private static final Set<String> ALL_ACTIONS = 
Sets.newHashSet(AccessConstants.ALL,
       AccessConstants.SELECT, AccessConstants.INSERT, AccessConstants.ALTER,
@@ -131,12 +126,11 @@ public class SentryStore {
     // CREDENTIAL_PROVIDER_PATH("hadoop.security.credential.provider.path" in 
sentry-site.xml
     // it falls back to reading directly from sentry-site.xml
     char[] passTmp = conf.getPassword(ServerConfig.SENTRY_STORE_JDBC_PASS);
-    String pass = null;
-    if(passTmp != null) {
-      pass = new String(passTmp);
-    } else {
-      throw new SentrySiteConfigurationException("Error reading " + 
ServerConfig.SENTRY_STORE_JDBC_PASS);
+    if (passTmp == null) {
+      throw new SentrySiteConfigurationException("Error reading " +
+              ServerConfig.SENTRY_STORE_JDBC_PASS);
     }
+    String pass = new String(passTmp);
 
     String driverName = conf.get(ServerConfig.SENTRY_STORE_JDBC_DRIVER,
         ServerConfig.SENTRY_STORE_JDBC_DRIVER_DEFAULT);
@@ -187,7 +181,7 @@ public class SentryStore {
   }
 
   // ensure that the backend DB schema is set
-  public void verifySentryStoreSchema(boolean checkVersion) throws Exception {
+  void verifySentryStoreSchema(boolean checkVersion) throws Exception {
     if (!checkVersion) {
       setSentryVersion(SentryStoreSchemaInfo.getSentryVersion(),
           "Schema version set implicitly");
@@ -216,7 +210,7 @@ public class SentryStore {
     }
   }
 
-  public void rollbackTransaction(PersistenceManager pm) {
+  private void rollbackTransaction(PersistenceManager pm) {
     if (pm == null || pm.isClosed()) {
       return;
     }
@@ -229,55 +223,83 @@ public class SentryStore {
       }
     }
   }
+
   /**
-  Get the MSentry object from roleName
-  Note: Should be called inside a transaction
+   * Get a single role with the given name inside a transaction
+   * @param pm Persistence Manager instance
+   * @param roleName Role name (should not be null)
+   * @return single role with the given name
    */
-  public MSentryRole getMSentryRole(PersistenceManager pm, String roleName) {
+  public MSentryRole getRole(PersistenceManager pm, String roleName) {
     Query query = pm.newQuery(MSentryRole.class);
-    query.setFilter("this.roleName == t");
-    query.declareParameters("java.lang.String t");
+    query.setFilter("this.roleName == :roleName");
     query.setUnique(true);
     return (MSentryRole) query.execute(roleName);
   }
 
   /**
-   * Normalize the string values
+   * Get list of all roles. Should be called inside transaction.
+   * @param pm Persistence manager instance
+   * @return List of all roles
+   */
+  @SuppressWarnings("unchecked")
+  private List<MSentryRole> getAllRoles(PersistenceManager pm) {
+    Query query = pm.newQuery(MSentryRole.class);
+    return (List<MSentryRole>) query.execute();
+  }
+
+  /**
+   * Get a group with the given name. Should be called inside transaction.
+   * @param pm Persistence Manager instance
+   * @param groupName - group name
+   * @return Single group with the given name
+     */
+  private MSentryGroup getGroup(PersistenceManager pm, String groupName) {
+    Query query = pm.newQuery(MSentryGroup.class);
+    query.setFilter("this.groupName == :groupName");
+    query.setUnique(true);
+    return (MSentryGroup) query.execute(groupName);
+  }
+
+  /**
+   * Normalize the string values - remove leading and trailing whitespaces and
+   * convert to lower case
+   * @return normalized input
    */
   private String trimAndLower(String input) {
     return input.trim().toLowerCase();
   }
   /**
-   * Create a sentry role and persist it.
-   * @param roleName: Name of the role being persisted
+   * Create a sentry role and persist it. Role name is the primary key for the
+   * role, so an attempt to create a role which exists fails with JDO 
exception.
+   *
+   * @param roleName: Name of the role being persisted.
+   *    The name is normalized.
    * @throws Exception
    */
   public void createSentryRole(final String roleName) throws Exception {
     tm.executeTransactionWithRetry(
         new TransactionBlock() {
           public Object execute(PersistenceManager pm) throws Exception {
-            createSentryRoleCore(pm, roleName);
+            String trimmedRoleName = trimAndLower(roleName);
+            if (getRole(pm, trimmedRoleName) != null) {
+              throw new SentryAlreadyExistsException("Role: " + 
trimmedRoleName);
+            }
+            pm.makePersistent(new MSentryRole(trimmedRoleName));
             return null;
             }
         });
   }
 
-  private void createSentryRoleCore(PersistenceManager pm, String roleName)
-      throws SentryAlreadyExistsException {
-    String trimmedRoleName = trimAndLower(roleName);
-    MSentryRole mSentryRole = getMSentryRole(pm, trimmedRoleName);
-    if (mSentryRole == null) {
-      MSentryRole mRole = new MSentryRole(trimmedRoleName, 
System.currentTimeMillis());
-      pm.makePersistent(mRole);
-    } else {
-      throw new SentryAlreadyExistsException("Role: " + trimmedRoleName);
-    }
-  }
-
+  /**
+   * Get count of object of the given class
+   * @param tClass Class to count
+   * @param <T> Class type
+   * @return count of objects or -1 in case of error
+     */
   private <T> Long getCount(final Class<T> tClass) {
-    Long size;
     try {
-      size = (Long) tm.executeTransaction(
+      return (Long) tm.executeTransaction(
           new TransactionBlock() {
             public Object execute(PersistenceManager pm) throws Exception {
               Query query = pm.newQuery();
@@ -287,10 +309,13 @@ public class SentryStore {
             }
           });
     } catch (Exception e) {
-      size = Long.valueOf(-1);
+       return COUNT_VALUE_UNKNOWN;
     }
-    return size;
   }
+
+  /**
+   * @return number of roles
+   */
   public Gauge<Long> getRoleCountGauge() {
     return new Gauge< Long >() {
       @Override
@@ -300,6 +325,9 @@ public class SentryStore {
     };
   }
 
+  /**
+   * @return Number of privileges
+   */
   public Gauge<Long> getPrivilegeCountGauge() {
     return new Gauge< Long >() {
       @Override
@@ -309,6 +337,9 @@ public class SentryStore {
     };
   }
 
+  /**
+   * @return number of groups
+   */
   public Gauge<Long> getGroupCountGauge() {
     return new Gauge< Long >() {
       @Override
@@ -318,6 +349,9 @@ public class SentryStore {
     };
   }
 
+  /**
+   * @return Number of users
+   */
   public Gauge<Long> getUserCountGauge() {
     return new Gauge<Long>() {
       @Override
@@ -356,12 +390,26 @@ public class SentryStore {
     }
   }
 
+  /**
+   * Grant privilege for a role
+   * @param grantorPrincipal User name
+   * @param roleName Role name
+   * @param privilege Privilege to grant
+   * @throws Exception
+   */
   public void alterSentryRoleGrantPrivilege(String grantorPrincipal,
       String roleName, TSentryPrivilege privilege) throws Exception {
     alterSentryRoleGrantPrivileges(grantorPrincipal, roleName,
             Sets.newHashSet(privilege));
   }
 
+  /**
+   * Grant multiple privileges
+   * @param grantorPrincipal User name
+   * @param roleName Role name
+   * @param privileges Set of privileges
+   * @throws Exception
+   */
   public void alterSentryRoleGrantPrivileges(final String grantorPrincipal,
       final String roleName, final Set<TSentryPrivilege> privileges) throws 
Exception {
     tm.executeTransactionWithRetry(
@@ -386,7 +434,7 @@ public class SentryStore {
       String roleName, TSentryPrivilege privilege)
       throws SentryNoSuchObjectException, SentryInvalidInputException {
     MSentryPrivilege mPrivilege = null;
-    MSentryRole mRole = getMSentryRole(pm, roleName);
+    MSentryRole mRole = getRole(pm, roleName);
     if (mRole == null) {
       throw new SentryNoSuchObjectException("Role: " + roleName + " doesn't 
exist");
     } else {
@@ -465,40 +513,34 @@ public class SentryStore {
   private void alterSentryRoleRevokePrivilegeCore(PersistenceManager pm,
       String roleName, TSentryPrivilege tPrivilege)
       throws SentryNoSuchObjectException, SentryInvalidInputException {
-    Query query = pm.newQuery(MSentryRole.class);
-    query.setFilter("this.roleName == t");
-    query.declareParameters("java.lang.String t");
-    query.setUnique(true);
-    MSentryRole mRole = (MSentryRole) query.execute(roleName);
+    MSentryRole mRole = getRole(pm, roleName);
     if (mRole == null) {
       throw new SentryNoSuchObjectException("Role: " + roleName + " doesn't 
exist");
+    }
+    MSentryPrivilege mPrivilege = getMSentryPrivilege(tPrivilege, pm);
+    if (mPrivilege == null) {
+      mPrivilege = convertToMSentryPrivilege(tPrivilege);
     } else {
-      query = pm.newQuery(MSentryPrivilege.class);
-      MSentryPrivilege mPrivilege = getMSentryPrivilege(tPrivilege, pm);
-      if (mPrivilege == null) {
-        mPrivilege = convertToMSentryPrivilege(tPrivilege);
-      } else {
-        mPrivilege = (MSentryPrivilege) pm.detachCopy(mPrivilege);
-      }
+      mPrivilege = pm.detachCopy(mPrivilege);
+    }
 
-      Set<MSentryPrivilege> privilegeGraph = Sets.newHashSet();
-      if (mPrivilege.getGrantOption() != null) {
-        privilegeGraph.add(mPrivilege);
-      } else {
-        MSentryPrivilege mTure = new MSentryPrivilege(mPrivilege);
-        mTure.setGrantOption(true);
-        privilegeGraph.add(mTure);
-        MSentryPrivilege mFalse = new MSentryPrivilege(mPrivilege);
-        mFalse.setGrantOption(false);
-        privilegeGraph.add(mFalse);
-      }
-      // Get the privilege graph
-      populateChildren(pm, Sets.newHashSet(roleName), mPrivilege, 
privilegeGraph);
-      for (MSentryPrivilege childPriv : privilegeGraph) {
-        revokePrivilegeFromRole(pm, tPrivilege, mRole, childPriv);
-      }
-      pm.makePersistent(mRole);
+    Set<MSentryPrivilege> privilegeGraph = Sets.newHashSet();
+    if (mPrivilege.getGrantOption() != null) {
+      privilegeGraph.add(mPrivilege);
+    } else {
+      MSentryPrivilege mTure = new MSentryPrivilege(mPrivilege);
+      mTure.setGrantOption(true);
+      privilegeGraph.add(mTure);
+      MSentryPrivilege mFalse = new MSentryPrivilege(mPrivilege);
+      mFalse.setGrantOption(false);
+      privilegeGraph.add(mFalse);
+    }
+    // Get the privilege graph
+    populateChildren(pm, Sets.newHashSet(roleName), mPrivilege, 
privilegeGraph);
+    for (MSentryPrivilege childPriv : privilegeGraph) {
+      revokePrivilegeFromRole(pm, tPrivilege, mRole, childPriv);
     }
+    pm.makePersistent(mRole);
   }
 
   /**
@@ -614,6 +656,7 @@ public class SentryStore {
     }
   }
 
+  @SuppressWarnings("unchecked")
   private Set<MSentryPrivilege> getChildPrivileges(PersistenceManager pm, 
Set<String> roleNames,
       MSentryPrivilege parent) throws SentryInvalidInputException {
     // Column and URI do not have children
@@ -661,6 +704,7 @@ public class SentryStore {
     return privileges;
   }
 
+  @SuppressWarnings("unchecked")
   private List<MSentryPrivilege> getMSentryPrivileges(TSentryPrivilege tPriv, 
PersistenceManager pm) {
     Query query = pm.newQuery(MSentryPrivilege.class);
     StringBuilder filters = new StringBuilder("this.serverName == \""
@@ -701,11 +745,7 @@ public class SentryStore {
     } else if (tPriv.getGrantOption().equals(TSentryGrantOption.FALSE)) {
       grantOption = false;
     }
-    Object obj = query.execute(grantOption);
-    if (obj != null) {
-      return (MSentryPrivilege) obj;
-    }
-    return null;
+    return (MSentryPrivilege)query.execute(grantOption);
   }
 
   public void dropSentryRole(final String roleName) throws Exception {
@@ -721,22 +761,16 @@ public class SentryStore {
   private void dropSentryRoleCore(PersistenceManager pm, String roleName)
       throws SentryNoSuchObjectException {
     String lRoleName = trimAndLower(roleName);
-    Query query = pm.newQuery(MSentryRole.class);
-    query.setFilter("this.roleName == t");
-    query.declareParameters("java.lang.String t");
-    query.setUnique(true);
-    MSentryRole sentryRole = (MSentryRole) query.execute(lRoleName);
+    MSentryRole sentryRole = getRole(pm, lRoleName);
     if (sentryRole == null) {
       throw new SentryNoSuchObjectException("Role: " + lRoleName + " doesn't 
exist");
-    } else {
-      pm.retrieve(sentryRole);
-      int numPrivs = sentryRole.getPrivileges().size();
-      sentryRole.removePrivileges();
-      // with SENTRY-398 generic model
-      sentryRole.removeGMPrivileges();
-      privCleaner.incPrivRemoval(numPrivs);
-      pm.deletePersistent(sentryRole);
     }
+    int numPrivs = sentryRole.getPrivileges().size();
+    sentryRole.removePrivileges();
+    // with SENTRY-398 generic model
+    sentryRole.removeGMPrivileges();
+    privCleaner.incPrivRemoval(numPrivs);
+    pm.deletePersistent(sentryRole);
   }
 
   public void alterSentryRoleAddGroups(final String grantorPrincipal,
@@ -753,30 +787,25 @@ public class SentryStore {
   private void alterSentryRoleAddGroupsCore(PersistenceManager pm, String 
roleName,
       Set<TSentryGroup> groupNames) throws SentryNoSuchObjectException {
     String lRoleName = trimAndLower(roleName);
-    Query query = pm.newQuery(MSentryRole.class);
-    query.setFilter("this.roleName == t");
-    query.declareParameters("java.lang.String t");
-    query.setUnique(true);
-    MSentryRole role = (MSentryRole) query.execute(lRoleName);
+    MSentryRole role = getRole(pm, lRoleName);
     if (role == null) {
       throw new SentryNoSuchObjectException("Role: " + lRoleName + " doesn't 
exist");
-    } else {
-      query = pm.newQuery(MSentryGroup.class);
-      query.setFilter("this.groupName == t");
-      query.declareParameters("java.lang.String t");
-      query.setUnique(true);
-      List<MSentryGroup> groups = Lists.newArrayList();
-      for (TSentryGroup tGroup : groupNames) {
-        String groupName = tGroup.getGroupName().trim();
-        MSentryGroup group = (MSentryGroup) query.execute(groupName);
-        if (group == null) {
-          group = new MSentryGroup(groupName, System.currentTimeMillis(), 
Sets.newHashSet(role));
-        }
-        group.appendRole(role);
-        groups.add(group);
+    }
+    Query query = pm.newQuery(MSentryGroup.class);
+    query.setFilter("this.groupName == t");
+    query.declareParameters("java.lang.String t");
+    query.setUnique(true);
+    List<MSentryGroup> groups = Lists.newArrayList();
+    for (TSentryGroup tGroup : groupNames) {
+      String groupName = tGroup.getGroupName().trim();
+      MSentryGroup group = (MSentryGroup) query.execute(groupName);
+      if (group == null) {
+        group = new MSentryGroup(groupName, System.currentTimeMillis(), 
Sets.newHashSet(role));
       }
-      pm.makePersistentAll(groups);
+      group.appendRole(role);
+      groups.add(group);
     }
+    pm.makePersistentAll(groups);
   }
 
   public void alterSentryRoleAddUsers(final String roleName,
@@ -793,26 +822,25 @@ public class SentryStore {
   private void alterSentryRoleAddUsersCore(PersistenceManager pm, String 
roleName,
       Set<String> userNames) throws SentryNoSuchObjectException {
     String trimmedRoleName = trimAndLower(roleName);
-    MSentryRole role = getMSentryRole(pm, trimmedRoleName);
+    MSentryRole role = getRole(pm, trimmedRoleName);
     if (role == null) {
       throw new SentryNoSuchObjectException("Role: " + trimmedRoleName);
-    } else {
-      Query query = pm.newQuery(MSentryUser.class);
-      query.setFilter("this.userName == t");
-      query.declareParameters("java.lang.String t");
-      query.setUnique(true);
-      List<MSentryUser> users = Lists.newArrayList();
-      for (String userName : userNames) {
-        userName = userName.trim();
-        MSentryUser user = (MSentryUser) query.execute(userName);
-        if (user == null) {
-          user = new MSentryUser(userName, System.currentTimeMillis(), 
Sets.newHashSet(role));
-        }
-        user.appendRole(role);
-        users.add(user);
+    }
+    Query query = pm.newQuery(MSentryUser.class);
+    query.setFilter("this.userName == t");
+    query.declareParameters("java.lang.String t");
+    query.setUnique(true);
+    List<MSentryUser> users = Lists.newArrayList();
+    for (String userName : userNames) {
+      userName = userName.trim();
+      MSentryUser user = (MSentryUser) query.execute(userName);
+      if (user == null) {
+        user = new MSentryUser(userName, System.currentTimeMillis(), 
Sets.newHashSet(role));
       }
-      pm.makePersistentAll(users);
+      user.appendRole(role);
+      users.add(user);
     }
+    pm.makePersistentAll(users);
   }
 
   public void alterSentryRoleDeleteUsers(final String roleName,
@@ -821,7 +849,7 @@ public class SentryStore {
         new TransactionBlock() {
           public Object execute(PersistenceManager pm) throws Exception {
             String trimmedRoleName = trimAndLower(roleName);
-            MSentryRole role = getMSentryRole(pm, trimmedRoleName);
+            MSentryRole role = getRole(pm, trimmedRoleName);
             if (role == null) {
               throw new SentryNoSuchObjectException("Role: " + 
trimmedRoleName);
             } else {
@@ -851,29 +879,24 @@ public class SentryStore {
         new TransactionBlock() {
           public Object execute(PersistenceManager pm) throws Exception {
             String trimmedRoleName = trimAndLower(roleName);
-            Query query = pm.newQuery(MSentryRole.class);
-            query.setFilter("this.roleName == t");
-            query.declareParameters("java.lang.String t");
-            query.setUnique(true);
-            MSentryRole role = (MSentryRole) query.execute(trimmedRoleName);
+            MSentryRole role = getRole(pm, trimmedRoleName);
             if (role == null) {
               throw new SentryNoSuchObjectException("Role: " + trimmedRoleName 
+ " doesn't exist");
-            } else {
-              query = pm.newQuery(MSentryGroup.class);
-              query.setFilter("this.groupName == t");
-              query.declareParameters("java.lang.String t");
-              query.setUnique(true);
-              List<MSentryGroup> groups = Lists.newArrayList();
-              for (TSentryGroup tGroup : groupNames) {
-                String groupName = tGroup.getGroupName().trim();
-                MSentryGroup group = (MSentryGroup) query.execute(groupName);
-                if (group != null) {
-                  group.removeRole(role);
-                  groups.add(group);
-                }
+            }
+            Query query = pm.newQuery(MSentryGroup.class);
+            query.setFilter("this.groupName == t");
+            query.declareParameters("java.lang.String t");
+            query.setUnique(true);
+            List<MSentryGroup> groups = Lists.newArrayList();
+            for (TSentryGroup tGroup : groupNames) {
+              String groupName = tGroup.getGroupName().trim();
+              MSentryGroup group = (MSentryGroup) query.execute(groupName);
+              if (group != null) {
+                group.removeRole(role);
+                groups.add(group);
               }
-              pm.makePersistentAll(groups);
             }
+            pm.makePersistentAll(groups);
             return null;
           }
         });
@@ -885,15 +908,10 @@ public class SentryStore {
         new TransactionBlock() {
           public Object execute(PersistenceManager pm) throws Exception {
             String trimmedRoleName = trimAndLower(roleName);
-            Query query = pm.newQuery(MSentryRole.class);
-            query.setFilter("this.roleName == t");
-            query.declareParameters("java.lang.String t");
-            query.setUnique(true);
-            MSentryRole sentryRole = (MSentryRole) 
query.execute(trimmedRoleName);
+            MSentryRole sentryRole = getRole(pm, trimmedRoleName);
             if (sentryRole == null) {
-              throw new SentryNoSuchObjectException("Role: " + trimmedRoleName 
+ " doesn't exist");
-            } else {
-              pm.retrieve(sentryRole);
+              throw new SentryNoSuchObjectException("Role: " + trimmedRoleName 
+
+                      " doesn't exist");
             }
             return sentryRole;
           }
@@ -930,8 +948,9 @@ public class SentryStore {
     return result;
   }
 
-  List<MSentryPrivilege> getMSentryPrivileges(final Set<String> roleNames,
-      final TSentryAuthorizable authHierarchy) {
+  @SuppressWarnings("unchecked")
+  private List<MSentryPrivilege> getMSentryPrivileges(final Set<String> 
roleNames,
+                                                      final 
TSentryAuthorizable authHierarchy) {
     List<MSentryPrivilege> result = new ArrayList<MSentryPrivilege>();
     if (roleNames == null || roleNames.isEmpty()) {
       return result;
@@ -979,6 +998,7 @@ public class SentryStore {
     return result;
   }
 
+  @SuppressWarnings("unchecked")
   List<MSentryPrivilege> getMSentryPrivilegesByAuth(final Set<String> 
roleNames,
       final TSentryAuthorizable authHierarchy) {
     List<MSentryPrivilege> result = new ArrayList<MSentryPrivilege>();
@@ -1093,10 +1113,12 @@ public class SentryStore {
    * @param roleNames : roleNames to look up (required)
    * @param authHierarchy : filter push down based on auth hierarchy (optional)
    * @return : Set of thrift sentry privilege objects
-   * @throws SentryNoSuchObjectException
+   * @throws SentryInvalidInputException
    */
 
-  public Set<TSentryPrivilege> getTSentryPrivileges(Set<String> roleNames, 
TSentryAuthorizable authHierarchy) throws SentryInvalidInputException {
+  public Set<TSentryPrivilege> getTSentryPrivileges(Set<String> roleNames,
+                                                    TSentryAuthorizable 
authHierarchy)
+          throws SentryInvalidInputException {
     if (authHierarchy.getServer() == null) {
       throw new SentryInvalidInputException("serverName cannot be null !!");
     }
@@ -1112,7 +1134,7 @@ public class SentryStore {
     return convertToTSentryPrivileges(getMSentryPrivileges(roleNames, 
authHierarchy));
   }
 
-
+  @SuppressWarnings("unchecked")
   private Set<MSentryRole> getMSentryRolesByGroupName(final String groupName)
       throws Exception {
     return (Set<MSentryRole>) tm.executeTransaction(
@@ -1122,20 +1144,12 @@ public class SentryStore {
 
             //If no group name was specified, return all roles
             if (groupName == null) {
-              Query query = pm.newQuery(MSentryRole.class);
-              roles = new 
HashSet<MSentryRole>((List<MSentryRole>)query.execute());
+              roles = new HashSet<MSentryRole>(getAllRoles(pm));
             } else {
-              Query query = pm.newQuery(MSentryGroup.class);
-              MSentryGroup sentryGroup;
               String trimmedGroupName = groupName.trim();
-              query.setFilter("this.groupName == t");
-              query.declareParameters("java.lang.String t");
-              query.setUnique(true);
-              sentryGroup = (MSentryGroup) query.execute(trimmedGroupName);
+              MSentryGroup sentryGroup = getGroup(pm, trimmedGroupName);
               if (sentryGroup == null) {
                 throw new SentryNoSuchObjectException("Group: " + 
trimmedGroupName + " doesn't exist");
-              } else {
-                pm.retrieve(sentryGroup);
               }
               roles = sentryGroup.getRoles();
             }
@@ -1149,7 +1163,7 @@ public class SentryStore {
 
   /**
    * Gets sentry role objects for a given groupName from the persistence layer
-   * @param groupName : groupName to look up ( if null returns all roles for 
all groups)
+   * @param groupNames : set of groupNames to look up ( if null returns all 
roles for all groups)
    * @return : Set of thrift sentry role objects
    * @throws SentryNoSuchObjectException
    */
@@ -1169,6 +1183,7 @@ public class SentryStore {
     return convertToTSentryRoles(roleSet);
   }
 
+  @SuppressWarnings("unchecked")
   public Set<String> getRoleNamesForGroups(final Set<String> groups) {
     if (groups == null || groups.isEmpty()) {
       return ImmutableSet.of();
@@ -1192,6 +1207,7 @@ public class SentryStore {
     return convertToRoleNameSet(getRolesForGroups(pm, groups));
   }
 
+  @SuppressWarnings("unchecked")
   public Set<String> getRoleNamesForUsers(final Set<String> users) {
     if (users == null || users.isEmpty()) {
       return ImmutableSet.of();
@@ -1215,6 +1231,7 @@ public class SentryStore {
     return convertToRoleNameSet(getRolesForUsers(pm, users));
   }
 
+  @SuppressWarnings("unchecked")
   public Set<TSentryRole> getTSentryRolesByUserNames(final Set<String> users) {
     Set<TSentryRole> result = new HashSet<>();
 
@@ -1251,7 +1268,7 @@ public class SentryStore {
     return result;
   }
 
-  public Set<MSentryRole> getRolesForUsers(PersistenceManager pm, Set<String> 
users) {
+  private Set<MSentryRole> getRolesForUsers(PersistenceManager pm, Set<String> 
users) {
     Set<MSentryRole> result = Sets.newHashSet();
     if (users != null) {
       Query query = pm.newQuery(MSentryUser.class);
@@ -1268,7 +1285,7 @@ public class SentryStore {
     return result;
   }
 
-  public Set<String> listAllSentryPrivilegesForProvider(Set<String> groups, 
Set<String> users,
+  Set<String> listAllSentryPrivilegesForProvider(Set<String> groups, 
Set<String> users,
       TSentryActiveRoleSet roleSet) throws SentryInvalidInputException {
     return listSentryPrivilegesForProvider(groups, users, roleSet, null);
   }
@@ -1292,6 +1309,7 @@ public class SentryStore {
     return hasAnyServerPrivileges(rolesToQuery, server);
   }
 
+  @SuppressWarnings("unchecked")
   private Set<String> getRolesToQuery(final Set<String> groups, final 
Set<String> users,
       final TSentryActiveRoleSet roleSet) {
     Set<String> result = new HashSet<>();
@@ -1409,7 +1427,7 @@ public class SentryStore {
     return group;
   }
 
-  protected TSentryPrivilege convertToTSentryPrivilege(MSentryPrivilege 
mSentryPrivilege) {
+  TSentryPrivilege convertToTSentryPrivilege(MSentryPrivilege 
mSentryPrivilege) {
     TSentryPrivilege privilege = new TSentryPrivilege();
     convertToTSentryPrivilege(mSentryPrivilege, privilege);
     return privilege;
@@ -1468,12 +1486,12 @@ public class SentryStore {
     return s.trim().toLowerCase();
   }
 
-  public String getSentryVersion() throws Exception {
+  String getSentryVersion() throws Exception {
     MSentryVersion mVersion = getMSentryVersion();
     return mVersion.getSchemaVersion();
   }
 
-  public void setSentryVersion(final String newVersion, final String 
verComment)
+  void setSentryVersion(final String newVersion, final String verComment)
       throws Exception {
     tm.executeTransaction(
         new TransactionBlock() {
@@ -1691,7 +1709,7 @@ public class SentryStore {
     return Strings.isNullOrEmpty(s) ? NULL_COL : s;
   }
 
-  public static String fromNULLCol(String s) {
+  private static String fromNULLCol(String s) {
     return isNULL(s) ? "" : s;
   }
 
@@ -1701,11 +1719,13 @@ public class SentryStore {
 
   /**
    * Grant option check
-   * @param pm
-   * @param privilege
+   * @param pm Persistence manager instance
+   * @param grantorPrincipal User name
+   * @param privilege Privilege to check
    * @throws SentryUserException
    */
-  private void grantOptionCheck(PersistenceManager pm, String 
grantorPrincipal, TSentryPrivilege privilege)
+  private void grantOptionCheck(PersistenceManager pm, String grantorPrincipal,
+                                TSentryPrivilege privilege)
       throws SentryUserException {
     MSentryPrivilege mPrivilege = convertToMSentryPrivilege(privilege);
     if (grantorPrincipal == null) {
@@ -1717,7 +1737,7 @@ public class SentryStore {
     // if grantor is in adminGroup, don't need to do check
     Set<String> admins = getAdminGroups();
     boolean isAdminGroup = false;
-    if (groups != null && admins != null && !admins.isEmpty()) {
+    if (groups != null && !admins.isEmpty()) {
       for (String g : groups) {
         if (admins.contains(g)) {
           isAdminGroup = true;
@@ -1761,8 +1781,9 @@ public class SentryStore {
   }
 
   /**
-   * This returns a Mapping of AuthZObj(db/table) -> (Role -> permission)
+   * @return  Mapping of AuthZObj(db/table) -> (Role -> permission)
    */
+  @SuppressWarnings("unchecked")
   public Map<String, HashMap<String, String>> retrieveFullPrivilegeImage() {
     Map<String, HashMap<String, String>> result = new HashMap<>();
     try {
@@ -1808,8 +1829,9 @@ public class SentryStore {
   }
 
   /**
-   * This returns a Mapping of Role -> [Groups]
+   * @return Mapping of Role -> [Groups]
    */
+  @SuppressWarnings("unchecked")
   public Map<String, LinkedList<String>> retrieveFullRoleImage() {
     Map<String, LinkedList<String>> result = new HashMap<>();
     try {
@@ -1911,7 +1933,7 @@ public class SentryStore {
      * NOTIFY_THRESHOLD times this routine is called.
      * @param numDeletions The number of potentially orphaned privileges
      */
-    public void incPrivRemoval(int numDeletions) {
+    void incPrivRemoval(int numDeletions) {
       if (privCleanerThread != null) {
         try {
           lock.lock();
@@ -1928,7 +1950,7 @@ public class SentryStore {
     /**
      * Simple form of incPrivRemoval when only one privilege is deleted.
      */
-    public void incPrivRemoval() {
+    void incPrivRemoval() {
       incPrivRemoval(1);
     }
 
@@ -1936,7 +1958,7 @@ public class SentryStore {
      * Tell this thread to exit. Safe to call multiple times, as it just
      * notifies the run() loop to finish up.
      */
-    public void exit() {
+    void exit() {
       if (privCleanerThread != null) {
         lock.lock();
         try {
@@ -1958,6 +1980,7 @@ public class SentryStore {
      * 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";
@@ -2030,6 +2053,7 @@ public class SentryStore {
   }
 
   // get mapping datas for [group,role], [user,role] with the specific roles
+  @SuppressWarnings("unchecked")
   public List<Map<String, Set<String>>> getGroupUserRoleMapList(final 
Set<String> roleNames) {
     List<Map<String, Set<String>>> result = new ArrayList<>();
     try {
@@ -2106,11 +2130,14 @@ public class SentryStore {
   }
 
   // get all mapping data for [role,privilege]
-  public Map<String, Set<TSentryPrivilege>> getRoleNameTPrivilegesMap() throws 
Exception {
+  Map<String, Set<TSentryPrivilege>> getRoleNameTPrivilegesMap() throws 
Exception {
     return getRoleNameTPrivilegesMap(null, null);
   }
 
-  // get mapping data for [role,privilege] with the specific auth object
+  /**
+   * @return mapping data for [role,privilege] with the specific auth object
+   */
+  @SuppressWarnings("unchecked")
   public Map<String, Set<TSentryPrivilege>> getRoleNameTPrivilegesMap(final 
String dbName,
         final String tableName) throws Exception {
     return (Map<String, Set<TSentryPrivilege>>) tm.executeTransaction(
@@ -2157,37 +2184,48 @@ public class SentryStore {
     return rolePrivilegesMap;
   }
 
-  // Get the all exist role names, will return an empty set
-  // if no role names exist.
+  /**
+   * @return Set of all role names, or an empty set if no roles are defined
+   */
+  @SuppressWarnings("unchecked")
   public Set<String> getAllRoleNames() {
     Set<String> result = new HashSet<>();
     try {
-      result = (Set<String>) tm.executeTransaction(
+      return (Set<String>) tm.executeTransaction(
           new TransactionBlock() {
             public Object execute(PersistenceManager pm) throws Exception {
-              return getAllRoleNames(pm);
+              return getAllRoleNamesCore(pm);
             }
           });
     } catch (Exception e) {
       LOGGER.error(e.getMessage(), e);
+      return  new HashSet<>();
     }
-    return result;
   }
 
-  // get the all exist role names
-  private Set<String> getAllRoleNames(PersistenceManager pm) {
-    Query query = pm.newQuery(MSentryRole.class);
-    List<MSentryRole> mSentryRoles = (List<MSentryRole>) query.execute();
-    Set<String> existRoleNames = Sets.newHashSet();
+  /**
+   * Get set of all role names
+   * Should be executed inside transaction
+   * @param pm PersistenceManager instance
+   * @return Set of all role names, or an empty set if no roles are defined
+   */
+  private Set<String> getAllRoleNamesCore(PersistenceManager pm) {
+    List<MSentryRole> mSentryRoles = getAllRoles(pm);
+    Set<String> roleNames = Sets.newHashSet();
     if (mSentryRoles != null) {
       for (MSentryRole mSentryRole : mSentryRoles) {
-        existRoleNames.add(mSentryRole.getRoleName());
+        roleNames.add(mSentryRole.getRoleName());
       }
     }
-    return existRoleNames;
+    return roleNames;
   }
 
-  // get the all exist groups
+  /**
+   * Get all groups as a map from group name to group
+   * @param pm PersistenceManager instance
+   * @return map of group names to group data for each group
+   */
+  @SuppressWarnings("unchecked")
   private Map<String, MSentryGroup> getGroupNameTGroupMap(PersistenceManager 
pm) {
     Query query = pm.newQuery(MSentryGroup.class);
     List<MSentryGroup> mSentryGroups = (List<MSentryGroup>) query.execute();
@@ -2201,7 +2239,13 @@ public class SentryStore {
     return existGroupsMap;
   }
 
+  /**
+   * Get all users as a map from user name to user
+   * @param pm PersistenceManager instance
+   * @return map of user names to user data for each user
+   */
   // get the all exist users
+  @SuppressWarnings("unchecked")
   private Map<String, MSentryUser> getUserNameToUserMap(PersistenceManager pm) 
{
     Query query = pm.newQuery(MSentryUser.class);
     List<MSentryUser> users = (List<MSentryUser>) query.execute();
@@ -2215,7 +2259,12 @@ public class SentryStore {
     return existUsersMap;
   }
 
-  // get the all exist privileges
+  /**
+   * Returl list of all privileges
+   * @param pm PersistenceManager instance
+   * @return List of all privileges
+   */
+  @SuppressWarnings("unchecked")
   private List<MSentryPrivilege> getPrivilegesList(PersistenceManager pm) {
     Query query = pm.newQuery(MSentryPrivilege.class);
     List<MSentryPrivilege> resultList = (List<MSentryPrivilege>) 
query.execute();
@@ -2226,14 +2275,14 @@ public class SentryStore {
   }
 
   @VisibleForTesting
+  @SuppressWarnings("unchecked")
   protected Map<String, MSentryRole> getRolesMap() {
     Map<String, MSentryRole> result = new HashMap<>();
     try {
       result = (Map<String, MSentryRole>) tm.executeTransaction(
           new TransactionBlock() {
             public Object execute(PersistenceManager pm) throws Exception {
-              Query query = pm.newQuery(MSentryRole.class);
-              List<MSentryRole> mSentryRoles = (List<MSentryRole>) 
query.execute();
+              List<MSentryRole> mSentryRoles = getAllRoles(pm);
               Map<String, MSentryRole> existRolesMap = Maps.newHashMap();
               if (mSentryRoles != null) {
                 // change the List<MSentryRole> -> Map<roleName, 
Set<MSentryRole>>
@@ -2252,6 +2301,7 @@ public class SentryStore {
   }
 
   @VisibleForTesting
+  @SuppressWarnings("unchecked")
   protected Map<String, MSentryGroup> getGroupNameToGroupMap() {
     Map<String, MSentryGroup>result = new HashMap<>();
     try {
@@ -2268,6 +2318,7 @@ public class SentryStore {
   }
 
   @VisibleForTesting
+  @SuppressWarnings("unchecked")
   protected Map<String, MSentryUser> getUserNameToUserMap() {
     Map<String, MSentryUser> result = new HashMap<>();
     try {
@@ -2284,6 +2335,7 @@ public class SentryStore {
   }
 
   @VisibleForTesting
+  @SuppressWarnings("unchecked")
   protected List<MSentryPrivilege> getPrivilegesList() {
     List<MSentryPrivilege> result = new ArrayList<>();
     try {
@@ -2342,8 +2394,8 @@ public class SentryStore {
           public Object execute(PersistenceManager pm) throws Exception {
             // change all role name in lowercase
             TSentryMappingData mappingData = 
lowercaseRoleName(tSentryMappingData);
-            Set<String> existRoleNames = getAllRoleNames(pm);
-            //
+            Set<String> roleNames = getAllRoleNamesCore(pm);
+
             Map<String, Set<TSentryGroup>> importedRoleGroupsMap = 
covertToRoleNameTGroupsMap(mappingData
                 .getGroupRolesMap());
             Map<String, Set<String>> importedRoleUsersMap = 
covertToRoleUsersMap(mappingData
@@ -2351,17 +2403,17 @@ public class SentryStore {
             Set<String> importedRoleNames = importedRoleGroupsMap.keySet();
             // if import with overwrite role, drop the duplicated roles in 
current DB first.
             if (isOverwriteForRole) {
-              dropDuplicatedRoleForImport(pm, existRoleNames, 
importedRoleNames);
-              // refresh the existRoleNames for the drop role
-              existRoleNames = getAllRoleNames(pm);
+              dropDuplicatedRoleForImport(pm, roleNames, importedRoleNames);
+              // refresh the roleNames for the drop role
+              roleNames = getAllRoleNamesCore(pm);
             }
 
-            // import the mapping data for [role,privilege], the 
existRoleNames will be updated
-            importRolePrivilegeMapping(pm, existRoleNames, 
mappingData.getRolePrivilegesMap());
-            // import the mapping data for [role,group], the existRoleNames 
will be updated
-            importRoleGroupMapping(pm, existRoleNames, importedRoleGroupsMap);
-            // import the mapping data for [role,user], the existRoleNames 
will be updated
-            importRoleUserMapping(pm, existRoleNames, importedRoleUsersMap);
+            // import the mapping data for [role,privilege], the roleNames 
will be updated
+            importRolePrivilegeMapping(pm, roleNames, 
mappingData.getRolePrivilegesMap());
+            // import the mapping data for [role,group], the roleNames will be 
updated
+            importRoleGroupMapping(pm, roleNames, importedRoleGroupsMap);
+            // import the mapping data for [role,user], the roleNames will be 
updated
+            importRoleUserMapping(pm, roleNames, importedRoleUsersMap);
             return null;
           }
         });
@@ -2493,9 +2545,10 @@ public class SentryStore {
     String lowerRoleName = trimAndLower(roleName);
     // if the rolenName doesn't exist, create it.
     if (!existRoleNames.contains(lowerRoleName)) {
-      createSentryRoleCore(pm, lowerRoleName);
       // update the exist role name set
       existRoleNames.add(lowerRoleName);
+      // Create role in the persistent storage
+      pm.makePersistent(new MSentryRole(trimAndLower(roleName)));
     }
   }
 }

http://git-wip-us.apache.org/repos/asf/sentry/blob/d9ab452f/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
----------------------------------------------------------------------
diff --git 
a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
 
b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
index ef32ad4..efe7f97 100644
--- 
a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
+++ 
b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
@@ -59,7 +59,7 @@ public class TestDelegateSentryStore extends 
SentryStoreIntegrationBase{
     sentryStore.createRole(SEARCH, roleName1, grantor);
     try {
       sentryStore.createRole(SEARCH, roleName2, grantor);
-      fail("Fail to throw SentryAlreadyExistsException");
+      fail("Fail to throw Exception");
     } catch (SentryAlreadyExistsException e) {
       //ignore the exception
     }
@@ -71,7 +71,7 @@ public class TestDelegateSentryStore extends 
SentryStoreIntegrationBase{
     }
   }
 
-  @Test(expected=SentryAlreadyExistsException.class)
+  @Test(expected=Exception.class)
   public void testCreateDuplicateRole() throws Exception {
     String roleName = "test-dup-role";
     String grantor = "grantor";

http://git-wip-us.apache.org/repos/asf/sentry/blob/d9ab452f/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 66f0584..64df6a5 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
@@ -115,6 +115,41 @@ public class TestSentryStore extends org.junit.Assert {
     }
   }
 
+  /**
+   * Fail test if role already exists
+   * @param roleName Role name to checl
+   * @throws Exception
+   */
+  private void checkRoleDoesNotExist(String roleName) throws Exception {
+    try {
+      sentryStore.getMSentryRoleByName(roleName);
+      fail("Role " + roleName + "already exists");
+    } catch (SentryNoSuchObjectException e) {
+      // Ok
+    }
+  }
+
+  /**
+   * Fail test if role doesn't exist
+   * @param roleName Role name to checl
+   * @throws Exception
+   */
+  private void checkRoleExists(String roleName) throws Exception {
+    assertEquals(roleName.toLowerCase(),
+            sentryStore.getMSentryRoleByName(roleName).getRoleName());
+  }
+
+  /**
+   * Create a role with the given name and verify that it is created
+   * @param roleName
+   * @throws Exception
+   */
+  private void createRole(String roleName) throws Exception {
+    checkRoleDoesNotExist(roleName);
+    sentryStore.createSentryRole(roleName);
+    checkRoleExists(roleName);
+  }
+
   @Test
   public void testCredentialProvider() throws Exception {
     assertArrayEquals(passwd, conf.getPassword(ServerConfig.
@@ -140,7 +175,8 @@ public class TestSentryStore extends org.junit.Assert {
 
     Set<String> users = Sets.newHashSet("user1");
 
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
+
     sentryStore.alterSentryRoleAddGroups(grantor, roleName, groups);
     sentryStore.alterSentryRoleDeleteGroups(roleName, groups);
     sentryStore.alterSentryRoleAddUsers(roleName, users);
@@ -154,7 +190,7 @@ public class TestSentryStore extends org.junit.Assert {
     String roleName = "test-dup-role";
     String grantor = "g1";
     String uri = 
"file:///var/folders/dt/9zm44z9s6bjfxbrm4v36lzdc0000gp/T/1401860678102-0/data/kv1.dat";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege tSentryPrivilege = new TSentryPrivilege("URI", "server1", 
"ALL");
     tSentryPrivilege.setURI(uri);
     sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, 
tSentryPrivilege);
@@ -200,7 +236,7 @@ public class TestSentryStore extends org.junit.Assert {
   @Test
   public void testCreateDuplicateRole() throws Exception {
     String roleName = "test-dup-role";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     try {
       sentryStore.createSentryRole(roleName);
       fail("Expected SentryAlreadyExistsException");
@@ -213,17 +249,22 @@ public class TestSentryStore extends org.junit.Assert {
   public void testCaseSensitiveScope() throws Exception {
     String roleName = "role1";
     String grantor = "g1";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege sentryPrivilege = new TSentryPrivilege("Database", 
"server1", "all");
     sentryPrivilege.setDbName("db1");
     sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, 
sentryPrivilege);
   }
 
+  /**
+   * Create a new role and then destroy it
+   * @throws Exception
+   */
   @Test
   public void testCreateDropRole() throws Exception {
     String roleName = "test-drop-role";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     sentryStore.dropSentryRole(roleName);
+    checkRoleDoesNotExist(roleName);
   }
 
   @Test
@@ -251,7 +292,7 @@ public class TestSentryStore extends org.junit.Assert {
   public void testAddDeleteGroups() throws Exception {
     String roleName = "test-groups";
     String grantor = "g1";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     Set<TSentryGroup> groups = Sets.newHashSet();
     TSentryGroup group = new TSentryGroup();
     group.setGroupName("test-groups-g1");
@@ -268,7 +309,7 @@ public class TestSentryStore extends org.junit.Assert {
   @Test
   public void testAddDeleteUsers() throws Exception {
     String roleName = "test-users";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     Set<String> users = Sets.newHashSet("test-user-u1", "test-user-u2");
     sentryStore.alterSentryRoleAddUsers(roleName, users);
     MSentryRole role = sentryStore.getMSentryRoleByName(roleName);
@@ -346,7 +387,7 @@ public class TestSentryStore extends org.junit.Assert {
     String server = "server1";
     String db = "db1";
     String table = "tbl1";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege privilege = new TSentryPrivilege();
     privilege.setPrivilegeScope("TABLE");
     privilege.setServerName(server);
@@ -402,7 +443,7 @@ public class TestSentryStore extends org.junit.Assert {
     final String dBase = "db";
     final String table = "table-";
 
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
 
     // Create NUM_PRIVS unique privilege objects in the database
     for (int i = 0; i < NUM_PRIVS; i++) {
@@ -444,7 +485,7 @@ public class TestSentryStore extends org.junit.Assert {
     final String dBase = "db";
     final String table = "table-";
 
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
 
     // Create NUM_PRIVS unique privilege objects in the database once more,
     // this time granting ALL and revoking SELECT to make INSERT.
@@ -483,7 +524,7 @@ public class TestSentryStore extends org.junit.Assert {
     String db = "db1";
     String table = "tbl1";
     String[] columns = {"c1","c2","c3","c4"};
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     Set<TSentryPrivilege> tPrivileges = Sets.newHashSet();
     for (String column : columns) {
       TSentryPrivilege privilege = new TSentryPrivilege();
@@ -544,7 +585,7 @@ public class TestSentryStore extends org.junit.Assert {
     String table = "tbl1";
     String column1 = "c1";
     String column2 = "c2";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege privilege = new TSentryPrivilege();
     privilege.setPrivilegeScope("COLUMN");
     privilege.setServerName(server);
@@ -620,7 +661,7 @@ public class TestSentryStore extends org.junit.Assert {
     String db = "db1";
     String table1 = "tbl1";
     String table2 = "tbl2";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege privilegeTable1 = new TSentryPrivilege();
     privilegeTable1.setPrivilegeScope("TABLE");
     privilegeTable1.setServerName(server);
@@ -694,7 +735,7 @@ public class TestSentryStore extends org.junit.Assert {
     String table = "tbl1";
     String column1 = "c1";
     String column2 = "c2";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege privilegeCol1 = new TSentryPrivilege();
     privilegeCol1.setPrivilegeScope("COLUMN");
     privilegeCol1.setServerName(server);
@@ -768,7 +809,7 @@ public class TestSentryStore extends org.junit.Assert {
     String db = "db1";
     String table = "tbl1";
     TSentryGrantOption grantOption = TSentryGrantOption.TRUE;
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
 
     TSentryPrivilege privilege = new TSentryPrivilege();
     privilege.setPrivilegeScope("TABLE");
@@ -789,7 +830,8 @@ public class TestSentryStore extends org.junit.Assert {
     assertEquals(0, privileges.size());
 
     roleName = "test-grantOption-db";
-    sentryStore.createSentryRole(roleName);
+
+    createRole(roleName);
     privilege = new TSentryPrivilege();
     privilege.setPrivilegeScope("DATABASE");
     privilege.setServerName(server);
@@ -1296,7 +1338,7 @@ public class TestSentryStore extends org.junit.Assert {
     String server = "server1";
     String db = "db1";
     String table = "tbl1";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege privilege = new TSentryPrivilege();
     privilege.setPrivilegeScope("TABLE");
     privilege.setServerName(server);
@@ -1952,7 +1994,7 @@ public class TestSentryStore extends org.junit.Assert {
     String grantor = "g1";
     String dbName = "db1";
     String table = "tb1";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege tSentryPrivilege = new TSentryPrivilege("TABLE", 
"server1", "ALL");
     tSentryPrivilege.setDbName(dbName);
     tSentryPrivilege.setTableName(table);
@@ -1991,7 +2033,7 @@ public class TestSentryStore extends org.junit.Assert {
     String dbName = "db1";
     String table = "tb1";
     String column = "col1";
-    sentryStore.createSentryRole(roleName);
+    createRole(roleName);
     TSentryPrivilege tSentryPrivilege = new TSentryPrivilege("TABLE", 
"server1", "ALL");
     tSentryPrivilege.setDbName(dbName);
     tSentryPrivilege.setTableName(table);

Reply via email to