sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kal...@apache.org
Subject sentry git commit: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot. (Kalyan Kumar Kalvagadda reviewed by Sergio Pena, Na Li and Arjun Mishra)
Date Fri, 14 Dec 2018 16:35:22 GMT
Repository: sentry
Updated Branches:
  refs/heads/master 41b090fbe -> d7fe39869


SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot. (Kalyan Kumar Kalvagadda reviewed
by Sergio Pena, Na Li and Arjun Mishra)


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

Branch: refs/heads/master
Commit: d7fe398693ac5bfc9e450c27e9afdc9d9de3dd62
Parents: 41b090f
Author: Kalyan Kumar Kalvagadda <kkalyan@cloudera.com>
Authored: Fri Dec 14 09:47:28 2018 -0600
Committer: Kalyan Kumar Kalvagadda <kkalyan@cloudera.com>
Committed: Fri Dec 14 10:31:37 2018 -0600

----------------------------------------------------------------------
 .../core/common/utils/SentryConstants.java      |   3 +
 .../sentry/service/common/ServiceConstants.java |   6 +
 .../db/service/model/MAuthzPathsMapping.java    | 164 ++++++++++++++++---
 .../sentry/provider/db/service/model/MPath.java |   8 +
 .../provider/db/service/model/package.jdo       |  28 +++-
 .../service/persistent/QueryParamBuilder.java   |  19 +++
 .../db/service/persistent/SentryStore.java      | 109 +++++++-----
 .../db/service/persistent/TestSentryStore.java  |  58 ++++++-
 8 files changed, 315 insertions(+), 80 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
----------------------------------------------------------------------
diff --git a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
index 9c2ba6f..8b53877 100644
--- a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
+++ b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
@@ -73,4 +73,7 @@ public class SentryConstants {
   // Representation for empty HMS snapshots not found on MAuthzPathsSnapshotId
   public static final long EMPTY_PATHS_SNAPSHOT_ID = 0L;
 
+  // Representation for empty HMS Objects on MAuthzPathsMapping
+  public static final long EMPTY_PATHS_MAPPING_ID = 0L;
+
 }

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
----------------------------------------------------------------------
diff --git a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
index 26a58f6..4508361 100644
--- a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
+++ b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
@@ -264,6 +264,12 @@ public class ServiceConstants {
      */
     public static final String SENTRY_DB_VALUE_GENERATION_ALLOCATION_SIZE = "sentry.db.valuegeneration.allocation.size";
     public static final int SENTRY_DB_VALUE_GENERATION_ALLOCATION_SIZE_DEFAULT = 100;
+
+    /**
+     * This value sets the maximum number of statements that can be included in a batch by
datanucleus
+     */
+    public static final String SENTRY_STATEMENT_BATCH_LIMIT = "sentry.statement.batch.limit";
+    public static final int SENTRY_STATEMENT_BATCH_LIMIT_DEFAULT = 100;
   }
 
   public static class ClientConfig {

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
index c51f25a..95f4b4b 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
@@ -18,34 +18,57 @@
 
 package org.apache.sentry.provider.db.service.model;
 
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+import javax.jdo.annotations.NotPersistent;
 import javax.jdo.annotations.PersistenceCapable;
+import java.util.Set;
+import java.util.HashSet;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.Collections;
+
+import org.apache.sentry.provider.db.service.persistent.QueryParamBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Transactional database backend for storing HMS Paths Updates. Any changes to this object
  * require re-running the maven build so DN can re-enhance.
+ *
  */
 @PersistenceCapable
 public class MAuthzPathsMapping {
+  private static final Logger LOGGER = LoggerFactory
+          .getLogger(MAuthzPathsMapping.class);
+
+  private long authzObjectId;
 
   private long authzSnapshotID;
   private String authzObjName;
-  private Set<MPath> paths;
+  // Used to fetch the paths from the sentry backend store.
+  private Set<MPath> pathsPersisted;
+  @NotPersistent
+  // Used to add paths.
+  private Set<String> pathsToPersist;
   private long createTimeMs;
 
-  public MAuthzPathsMapping(long authzSnapshotID, String authzObjName, Collection<String>
paths) {
+  public MAuthzPathsMapping(long authzSnapshotID, long authzObjectId, String authzObjName,
Collection<String> paths) {
     this.authzSnapshotID = authzSnapshotID;
+    this.authzObjectId = authzObjectId;
     this.authzObjName = MSentryUtil.safeIntern(authzObjName);
-    this.paths = new HashSet<>(paths.size());
-    for (String path : paths) {
-      this.paths.add(new MPath(path));
+    this.pathsPersisted = Collections.EMPTY_SET;
+    this.pathsToPersist = new HashSet<>(paths.size());
+    for(String path : paths) {
+      this.pathsToPersist.add(path);
     }
     this.createTimeMs = System.currentTimeMillis();
   }
 
+  public long getAuthzObjectId() {
+    return authzObjectId;
+  }
+
   public long getCreateTime() {
     return createTimeMs;
   }
@@ -62,20 +85,18 @@ public class MAuthzPathsMapping {
     this.authzObjName = authzObjName;
   }
 
-  public void setPaths(Set<MPath> paths) {
-    this.paths = paths;
-  }
-
-  public Set<MPath> getPaths() {
-    return paths;
-  }
-
-  public boolean removePath(MPath path) {
-    return paths.remove(path);
+  public Set<MPath> getPathsPersisted() {
+    return pathsPersisted;
   }
 
-  public void addPath(MPath path) {
-    paths.add(path);
+  public void addPathToPersist(Collection<String> paths) {
+    if(paths == null || paths.size() == 0) {
+      return;
+    }
+    if(pathsToPersist == null) {
+      pathsToPersist = new HashSet<>(paths.size());
+    }
+    pathsToPersist.addAll(paths);
   }
 
   /**
@@ -85,8 +106,8 @@ public class MAuthzPathsMapping {
    * @param path the given path name
    * @return an Path object that has the given path value.
    */
-  public MPath getPath(String path) {
-    for (MPath mPath : paths) {
+  public MPath getPathsPersisted(String path) {
+    for (MPath mPath : pathsPersisted) {
       if (mPath.getPath().equals(path)) {
         return mPath;
       }
@@ -98,8 +119,8 @@ public class MAuthzPathsMapping {
    * @return collection of paths strings contained in this object.
    */
   public Collection<String> getPathStrings() {
-    Collection<String> pathValues = new ArrayList<>(this.paths.size());
-    for (MPath path : this.paths) {
+    Collection<String> pathValues = new ArrayList<>(this.pathsPersisted.size());
+    for (MPath path : this.pathsPersisted) {
       pathValues.add(path.getPath());
     }
     return pathValues;
@@ -107,8 +128,10 @@ public class MAuthzPathsMapping {
 
   @Override
   public String toString() {
-    return "MSentryPathsUpdate authzSnapshotID=[" + authzSnapshotID + "], authzObj=[" + authzObjName
-        + "], paths=[" + paths.toString() + "], createTimeMs=[" + createTimeMs + "]";
+    return "MSentryPathsUpdate authzSnapshotID=[" + authzSnapshotID + "], authzObjectId=["
+ authzObjectId + "]," +
+            "authzObj=[" + authzObjName + "], mPaths=[" + ((pathsPersisted != null) ? pathsPersisted.toString()
: "")
+            + "], paths=[" + ((pathsToPersist != null) ? pathsToPersist.toString() : "")
+ "]," +
+            "createTimeMs=[" + createTimeMs + "]";
   }
 
   @Override
@@ -140,8 +163,97 @@ public class MAuthzPathsMapping {
       return false;
     } else if (authzSnapshotID != other.authzSnapshotID) {
       return false;
-   }
+    } else if (authzObjectId != other.authzObjectId) {
+      return false;
+    }
 
     return true;
   }
+
+  /**
+   * Persist the MAuthzPathsMapping by inserting entries into AUTHZ_PATH table in batches.
+   * @param pm Persistence manager instance
+   */
+  public void makePersistent(PersistenceManager pm) {
+    pm.makePersistent(this);
+    for (String path : pathsToPersist) {
+      pm.makePersistent(new MPathToPersist(authzObjectId, path));
+    }
+    pathsToPersist.clear();
+  }
+
+  /**
+   * Delete the paths associated with MAuthzPathsMapping entry.
+   * @param pm Persistence manager instance
+   * @param paths paths to be removed.
+   */
+  public void deletePersistent(PersistenceManager pm, Iterable<String> paths) {
+    Query query = pm.newQuery(MPathToPersist.class);
+    QueryParamBuilder paramBuilder = QueryParamBuilder.addPathFilter(null, paths).addObject("authzObjectId",
+        authzObjectId);
+    query.setFilter(paramBuilder.toString());
+    long delCount = query.deletePersistentAll(paramBuilder.getArguments());
+    LOGGER.debug("Entries deleted are %d", delCount);
+  }
+
+  /**
+  * There are performance issues in persisting instances of MPath at bulk because of the
FK mapping with
+  *  MAuthzPathsMapping. This FK mapping limits datanucleus from inserting these entries
in batches.
+  *  MPathToPersist is introduced to solve this performance issue.
+  *  Note: Only {@link MPathToPersist} should be used to persist paths instead of {@link
MPath} otherwise there
+  *  will be duplicate entry exceptions as both JDO's MPath and  {@link MPathToPersist}
+  *  maintain their own sequence for PATH_ID column.
+  */
+  @PersistenceCapable
+  public static class MPathToPersist {
+    private String path;
+    private long authzObjectId;
+
+
+    public MPathToPersist(long authzObjectId, String path) {
+      this.authzObjectId = authzObjectId;
+      this.path = MSentryUtil.safeIntern(path);
+    }
+
+    public String getPath() {
+      return path;
+    }
+
+    public void setPath(String path) {
+      this.path = path;
+    }
+
+    @Override
+    public int hashCode() {
+      final int prime = 31;
+      int result = 1;
+      result = prime * result + ((path == null) ? 0 : path.hashCode());
+      return result;
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      if (this == obj) {
+        return true;
+      }
+
+      if (obj == null) {
+        return false;
+      }
+
+      if (getClass() != obj.getClass()) {
+        return false;
+      }
+
+      MPathToPersist other = (MPathToPersist) obj;
+
+      if (path == null) {
+        return other.path == null;
+      } else if (authzObjectId != other.authzObjectId) {
+        return false;
+      }
+
+      return path.equals(other.path);
+    }
+  }
 }

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
index b0eaff2..ef2bf6c 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
@@ -22,6 +22,14 @@ package org.apache.sentry.provider.db.service.model;
  *
  * New class is created for path in order to have 1 to many mapping
  * between path and Authorizable object.
+ *
+ * There are performance issues in persisting instances of MPath at bulk because of the FK
mapping with
+ * MAuthzPathsMapping. This FK mapping limits datanucleus from inserting these entries in
batches.
+ * MPathToPersist was introduced to solve this performance issue.
+ * Note: This class should not be used to insert paths, instead {@link MAuthzPathsMapping.MPathToPersist}
should be used.
+ *  If MPath is used to persist paths, there will be duplicate entry exceptions as both JDO's
MPath and
+ *  {@link MAuthzPathsMapping.MPathToPersist} maintain their own sequence for PATH_ID column.
+
  */
 public class MPath {
   private String path;

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
index e3ae24b..4b27777 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
@@ -255,10 +255,10 @@
        </field>
     </class>
 
-    <class name="MAuthzPathsMapping" identity-type="datastore" table="AUTHZ_PATHS_MAPPING"
detachable="true">
-       <datastore-identity strategy="increment">
-         <column name="AUTHZ_OBJ_ID"/>
-       </datastore-identity>
+    <class name="MAuthzPathsMapping" identity-type="application" table="AUTHZ_PATHS_MAPPING"
detachable="true">
+       <field name="authzObjectId" primary-key="true">


+         <column name="AUTHZ_OBJ_ID" jdbc-type="BIGINT" allows-null="false"/>


+       </field>
        <index name="AUTHZ_SNAPSHOT_ID_INDEX" unique="false">
          <field name="authzSnapshotID"/>
        </index>
@@ -273,7 +273,7 @@
        <field name="createTimeMs">
          <column name="CREATE_TIME_MS" jdbc-type="BIGINT"/>
        </field>
-       <field name = "paths">
+       <field name = "pathsPersisted">
          <!-- Setting attribute dependent-element to true enables JDO cascading operations.
in this case we need it to
          cascade delete.
          -->
@@ -283,7 +283,7 @@
            </element>
        </field>
        <fetch-group name="includingPaths">
-         <field name="paths"/>
+         <field name="pathsPersisted"/>
        </fetch-group>
        <field name="authzSnapshotID">
          <column name="AUTHZ_SNAPSHOT_ID" jdbc-type="BIGINT" allows-null="false"/>
@@ -299,6 +299,22 @@
       </field>
     </class>
 
+    <!--
+    MPath has external foreign keys with MAuthzPathsMapping which limits it from persisting
+    in batches. MPathToPersist is replica of MPath with out the external foreign keys.
+    -->
+  <class name="MAuthzPathsMapping$MPathToPersist" identity-type="datastore" table="AUTHZ_PATH"
detachable="true">

+    <datastore-identity strategy="increment">

+      <column name="PATH_ID"/>

+    </datastore-identity>

+    <field name="authzObjectId">

+      <column name="AUTHZ_OBJ_ID" jdbc-type="BIGINT" allows-null="false"/>

+    </field>

+    <field name="path">

+      <column name="PATH_NAME" length="4000" jdbc-type="VARCHAR"/>

+    </field>
+  
</class>
+
      <class name="MSentryPermChange" table="SENTRY_PERM_CHANGE" identity-type="application"
detachable="true">
        <field name="changeID" primary-key="true">
          <column name="CHANGE_ID" jdbc-type="BIGINT" allows-null="false"/>

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
index f5802d7..11c208d 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
+++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
@@ -345,6 +345,25 @@ public class QueryParamBuilder {
   }
 
   /**
+   * Add common filter for set of paths. This is used to simplify creating filters for
+   * a collections of paths
+   * @param paramBuilder paramBuilder for parameters
+   * @param paths set paths
+   * @return paramBuilder supplied or a new one if the supplied one is null.
+   */
+  public static QueryParamBuilder addPathFilter(QueryParamBuilder paramBuilder,
+                                                 Iterable<String> paths) {
+    if (paramBuilder == null) {
+      paramBuilder = new QueryParamBuilder();
+    }
+    if (paths == null) {
+      return paramBuilder;
+    }
+    paramBuilder.newChild().addSet("this.path == ", paths, false);
+    return paramBuilder;
+  }
+
+  /**
    * Add multiple conditions for set of values.
    * <p>
    * Example:

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/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 dcf4651..63f5375 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
@@ -25,6 +25,7 @@ import static org.apache.sentry.core.common.utils.SentryConstants.DB_NAME;
 import static org.apache.sentry.core.common.utils.SentryConstants.EMPTY_CHANGE_ID;
 import static org.apache.sentry.core.common.utils.SentryConstants.EMPTY_NOTIFICATION_ID;
 import static org.apache.sentry.core.common.utils.SentryConstants.EMPTY_PATHS_SNAPSHOT_ID;
+import static org.apache.sentry.core.common.utils.SentryConstants.EMPTY_PATHS_MAPPING_ID;
 import static org.apache.sentry.core.common.utils.SentryConstants.GRANT_OPTION;
 import static org.apache.sentry.core.common.utils.SentryConstants.INDEX_GROUP_ROLES_MAP;
 import static org.apache.sentry.core.common.utils.SentryConstants.INDEX_USER_ROLES_MAP;
@@ -113,6 +114,7 @@ import static org.apache.sentry.core.common.utils.SentryConstants.TABLE_NAME;
 import static org.apache.sentry.core.common.utils.SentryConstants.URI;
 import static org.apache.sentry.core.common.utils.SentryUtils.isNULL;
 import static org.apache.sentry.hdfs.Updateable.Update;
+import static org.apache.sentry.service.common.ServiceConstants.ServerConfig.SENTRY_STATEMENT_BATCH_LIMIT;
 
 /**
  * SentryStore is the data access object for Sentry data. Strings
@@ -250,6 +252,10 @@ public class SentryStore implements SentryStoreInterface {
     // Disallow operations outside of transactions
     prop.setProperty("datanucleus.NontransactionalRead", "false");
     prop.setProperty("datanucleus.NontransactionalWrite", "false");
+    int batchSize = conf.getInt(SENTRY_STATEMENT_BATCH_LIMIT, ServerConfig.
+            SENTRY_STATEMENT_BATCH_LIMIT_DEFAULT);
+    prop.setProperty("datanucleus.rdbms.statementBatchLimit", Integer.toString(batchSize));
+
     int allocationSize = conf.getInt(ServerConfig.SENTRY_DB_VALUE_GENERATION_ALLOCATION_SIZE,
ServerConfig.
             SENTRY_DB_VALUE_GENERATION_ALLOCATION_SIZE_DEFAULT);
     prop.setProperty("datanucleus.valuegeneration.increment.allocationSize", Integer.toString(allocationSize));
@@ -3309,12 +3315,12 @@ public class SentryStore implements SentryStoreInterface {
 
               pm.setDetachAllOnCommit(false); // No need to detach objects
               deleteNotificationsSince(pm, notificationID + 1);
-
-              // persist the notidicationID
+              // persist the notification ID
               pm.makePersistent(new MSentryHmsNotification(notificationID));
 
               // persist the full snapshot
               long snapshotID = getCurrentAuthzPathsSnapshotID(pm);
+              long nextObjectId = getNextAuthzObjectID(pm);
               long nextSnapshotID = snapshotID + 1;
               pm.makePersistent(new MAuthzPathsSnapshotId(nextSnapshotID));
               LOGGER.info("Attempting to commit new HMS snapshot with ID = {}", nextSnapshotID);
@@ -3322,8 +3328,9 @@ public class SentryStore implements SentryStoreInterface {
               long lastProgressTime = System.currentTimeMillis();
 
               for (Map.Entry<String, Collection<String>> authzPath : authzPaths.entrySet())
{
-                pm.makePersistent(new MAuthzPathsMapping(nextSnapshotID, authzPath.getKey(),
authzPath.getValue()));
-
+                MAuthzPathsMapping mapping = new MAuthzPathsMapping(nextSnapshotID, nextObjectId++,
authzPath.getKey(),
+                        authzPath.getValue());
+                mapping.makePersistent(pm);
                 objectsPersistedCount++;
                 pathsPersistedCount = pathsPersistedCount + authzPath.getValue().size();
 
@@ -3355,6 +3362,17 @@ public class SentryStore implements SentryStoreInterface {
   }
 
   /**
+   * Get the Next object ID to be persisted
+   * Always executed in the transaction context.
+   *
+   * @param pm The PersistenceManager object.
+   * @return the Next object ID to be persisted. It returns 0 if no rows are found.
+   */
+  private static long getNextAuthzObjectID(PersistenceManager pm) {
+    return getMaxPersistedIDCore(pm, MAuthzPathsMapping.class, "authzObjectId", EMPTY_PATHS_MAPPING_ID)
+ 1;
+  }
+
+  /**
    * Get the last authorization path snapshot ID persisted.
    * Always executed in the transaction context.
    *
@@ -3373,7 +3391,8 @@ public class SentryStore implements SentryStoreInterface {
    *
    * @return the last persisted snapshot ID. It returns 0 if no rows are found.
    */
-  private long getCurrentAuthzPathsSnapshotID() throws Exception {
+  @VisibleForTesting
+  long getCurrentAuthzPathsSnapshotID() throws Exception {
     return tm.executeTransaction(
             SentryStore::getCurrentAuthzPathsSnapshotID
     );
@@ -3416,13 +3435,11 @@ public class SentryStore implements SentryStoreInterface {
 
     MAuthzPathsMapping mAuthzPathsMapping = getMAuthzPathsMappingCore(pm, currentSnapshotID,
authzObj);
     if (mAuthzPathsMapping == null) {
-      mAuthzPathsMapping = new MAuthzPathsMapping(currentSnapshotID, authzObj, paths);
+      mAuthzPathsMapping = new MAuthzPathsMapping(currentSnapshotID, getNextAuthzObjectID(pm),
authzObj, paths);
     } else {
-      for (String path : paths) {
-        mAuthzPathsMapping.addPath(new MPath(path));
-      }
+      mAuthzPathsMapping.addPathToPersist(paths);
     }
-    pm.makePersistent(mAuthzPathsMapping);
+    mAuthzPathsMapping.makePersistent(pm);
   }
 
   /**
@@ -3460,16 +3477,7 @@ public class SentryStore implements SentryStoreInterface {
 
     MAuthzPathsMapping mAuthzPathsMapping = getMAuthzPathsMappingCore(pm, currentSnapshotID,
authzObj);
     if (mAuthzPathsMapping != null) {
-      for (String path : paths) {
-        MPath mPath = mAuthzPathsMapping.getPath(path);
-        if (mPath == null) {
-          LOGGER.error("nonexistent path: {}", path);
-        } else {
-          mAuthzPathsMapping.removePath(mPath);
-          pm.deletePersistent(mPath);
-        }
-      }
-      pm.makePersistent(mAuthzPathsMapping);
+      mAuthzPathsMapping.deletePersistent(pm, paths);
     } else {
       LOGGER.error("nonexistent authzObj: {} on current paths snapshot ID #{}",
           authzObj, currentSnapshotID);
@@ -3557,16 +3565,10 @@ public class SentryStore implements SentryStoreInterface {
 
     MAuthzPathsMapping mAuthzPathsMapping = getMAuthzPathsMappingCore(pm, currentSnapshotID,
oldObj);
     if (mAuthzPathsMapping != null) {
-      MPath mOldPath = mAuthzPathsMapping.getPath(oldPath);
-      if (mOldPath == null) {
-        LOGGER.error("nonexistent path: {}", oldPath);
-      } else {
-        mAuthzPathsMapping.removePath(mOldPath);
-        pm.deletePersistent(mOldPath);
-      }
-      mAuthzPathsMapping.addPath(new MPath(newPath));
+      mAuthzPathsMapping.deletePersistent(pm,Collections.singleton(oldPath));
       mAuthzPathsMapping.setAuthzObjName(newObj);
-      pm.makePersistent(mAuthzPathsMapping);
+      mAuthzPathsMapping.addPathToPersist(Collections.singleton(newPath));
+      mAuthzPathsMapping.makePersistent(pm);
     } else {
       LOGGER.error("nonexistent authzObj: {} on current paths snapshot ID #{}",
           oldObj, currentSnapshotID);
@@ -3703,24 +3705,40 @@ public class SentryStore implements SentryStoreInterface {
 
     MAuthzPathsMapping mAuthzPathsMapping = getMAuthzPathsMappingCore(pm, currentSnapshotID,
authzObj);
     if (mAuthzPathsMapping == null) {
-      mAuthzPathsMapping = new MAuthzPathsMapping(currentSnapshotID, authzObj, Sets.newHashSet(newPath));
+      mAuthzPathsMapping = new MAuthzPathsMapping(currentSnapshotID, getNextAuthzObjectID(pm),
authzObj,
+              Collections.singleton(newPath));
     } else {
-      MPath mOldPath = mAuthzPathsMapping.getPath(oldPath);
-      if (mOldPath == null) {
-        LOGGER.error("nonexistent path: {}", oldPath);
-      } else {
-        mAuthzPathsMapping.removePath(mOldPath);
-        pm.deletePersistent(mOldPath);
-      }
-      MPath mNewPath = new MPath(newPath);
-      mAuthzPathsMapping.addPath(mNewPath);
+      mAuthzPathsMapping.deletePersistent(pm, Collections.singleton(oldPath));
+      mAuthzPathsMapping.addPathToPersist(Collections.singleton(newPath));
     }
-    pm.makePersistent(mAuthzPathsMapping);
+    mAuthzPathsMapping.makePersistent(pm);
   }
 
   /**
-   * Get the MAuthzPathsMapping object from authzObj
+   * Get the Collection of MPath associated with snapshot id and authzObj
+   * @param authzSnapshotID Snapshot ID
+   * @param authzObj Object name
+   * @return Path mapping for object provided.
+   * @throws Exception
    */
+  @VisibleForTesting
+   Set<MPath> getMAuthzPaths(long authzSnapshotID, String authzObj) throws Exception
{
+    return tm.executeTransactionWithRetry( pm -> {
+      MAuthzPathsMapping mapping = null;
+      pm.setDetachAllOnCommit(true); // No need to detach objects
+      mapping = getMAuthzPathsMappingCore(pm, authzSnapshotID, authzObj);
+      if(mapping != null) {
+        Set<MPath> paths = mapping.getPathsPersisted();
+        return paths;
+      } else {
+        return Collections.emptySet();
+      }
+    });
+  }
+
+    /**
+     * Get the MAuthzPathsMapping object from authzObj
+     */
   private MAuthzPathsMapping getMAuthzPathsMappingCore(PersistenceManager pm,
         long authzSnapshotID, String authzObj) {
     Query query = pm.newQuery(MAuthzPathsMapping.class);
@@ -3782,6 +3800,15 @@ public class SentryStore implements SentryStoreInterface {
   }
 
   /**
+   * Get the total number of entries in AUTHZ_PATH table.
+   * @return  number of entries in AUTHZ_PATH table.
+   */
+  @VisibleForTesting
+   long getPathCount() {
+    return getCount(MPath.class);
+  }
+
+  /**
    * Method detects orphaned privileges
    *
    * @return True, If there are orphan privileges

http://git-wip-us.apache.org/repos/asf/sentry/blob/d7fe3986/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 ca8c416..b327e9e 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
@@ -19,6 +19,7 @@
 package org.apache.sentry.provider.db.service.persistent;
 
 import java.io.File;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -39,6 +40,7 @@ import java.util.concurrent.atomic.AtomicLong;
 import com.google.common.collect.Lists;
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang.StringUtils;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.security.alias.CredentialProvider;
 import org.apache.hadoop.security.alias.CredentialProviderFactory;
@@ -2473,11 +2475,44 @@ public class TestSentryStore extends org.junit.Assert {
     assertTrue(CollectionUtils.isEqualCollection(Lists.newArrayList("/user/hive/warehouse/db2.db/table2.1",
         "/user/hive/warehouse/db2.db/table2.3"),
         pathImage.get("db2.table2")));
-    assertEquals(6, sentryStore.getMPaths().size());
+    assertEquals(6, sentryStore.getPathCount());
     assertEquals(notificationID, savedNotificationID);
   }
 
   @Test
+  public void testAddAuthzPathsMapping() throws Exception {
+    Set<MPath> paths;
+    // Persist an empty image so that we can add paths to it.
+    sentryStore.persistFullPathsImage(new HashMap<String, Collection<String>>(),
0);
+
+    // Check the latest persisted ID matches to both the path updates
+    long latestID = sentryStore.getCurrentAuthzPathsSnapshotID();
+
+
+    // Persist the path
+    sentryStore.addAuthzPathsMapping("db1.tb1", Arrays.asList("/hive/db1/tb1"), null);
+    paths = sentryStore.getMAuthzPaths(latestID, "db1.tb1");
+    // Verify that mapping is been added
+    assertEquals(paths.size(), 1);
+    assertTrue(paths.stream().anyMatch(x -> x.getPath().equalsIgnoreCase("/hive/db1/tb1")));
+
+    // Add new path to an existing mapping
+    sentryStore.addAuthzPathsMapping("db1.tb1", Arrays.asList("/hive/db1/tb1/par1"), null);
+    paths = sentryStore.getMAuthzPaths(latestID, "db1.tb1");
+    // Verify that mapping is been updated with the new path
+    assertEquals(paths.size(), 2);
+    assertTrue(paths.stream().anyMatch(x -> x.getPath().equalsIgnoreCase("/hive/db1/tb1/par1")));
+
+    // Add multiples path to an existing mapping
+    sentryStore.addAuthzPathsMapping("db1.tb1", Arrays.asList("/hive/db1/tb1/par2","/hive/db1/tb1/par3"),
null);
+    paths = sentryStore.getMAuthzPaths(latestID, "db1.tb1");
+    // Verify that mapping is been updated with the new path
+    assertEquals(paths.size(), 4);
+    assertTrue(paths.stream().anyMatch(x -> x.getPath().equalsIgnoreCase("/hive/db1/tb1/par2")));
+    assertTrue(paths.stream().anyMatch(x -> x.getPath().equalsIgnoreCase("/hive/db1/tb1/par3")));
+  }
+
+  @Test
   public void testAddPathsWithDuplicatedNotificationIdShouldBeAllowed() throws Exception
{
     long notificationID = 1;
 
@@ -4142,30 +4177,39 @@ public class TestSentryStore extends org.junit.Assert {
   @Test
   public void testPersistFullPathsImageWithHugeData() throws Exception {
     Map<String, Collection<String>> authzPaths = new HashMap<>();
-    String[] prefixes = {"/user/hive/warehouse"};
+    String[] prefixes = {"/user/hive/warehouse/"};
+    int dbCount = 10, tableCount = 10, partitionCount = 10, prefixSize;
+    prefixSize = StringUtils.countMatches(prefixes[0], "/");
+
     // Makes sure that authorizable object could be associated
     // with different paths and can be properly persisted into database.
-    for(int db_index = 1 ; db_index <= 10; db_index++) {
+    for(int db_index = 1 ; db_index <= dbCount; db_index++) {
       String db_name = "db" + db_index;
-      for( int table_index = 1; table_index <= 15; table_index++) {
+      for( int table_index = 1; table_index <= tableCount; table_index++) {
         Set<String> paths = Sets.newHashSet();
         String table_name = "tb" + table_index;
-        String location = "/u/h/w/" + db_name + "/" + table_name;
-        for (int part_index = 1; part_index <= 30; part_index++) {
+        String location = prefixes[0] + db_name + "/" + table_name;
+        paths.add(location);
+        for (int part_index = 1; part_index <= partitionCount; part_index++) {
           paths.add(location + "/" + part_index);
         }
         authzPaths.put(db_name+table_name, paths);
       }
     }
     long notificationID = 110000;
+    LOGGER.debug("Before " + Instant.now());
     sentryStore.persistFullPathsImage(authzPaths, notificationID);
+    LOGGER.debug("After " + Instant.now());
     PathsUpdate pathsUpdate = sentryStore.retrieveFullPathsImageUpdate(prefixes);
+    assertTrue(pathsUpdate.hasFullImage());
     long savedNotificationID = sentryStore.getLastProcessedNotificationID();
     assertEquals(1, pathsUpdate.getImgNum());
     TPathsDump pathDump = pathsUpdate.toThrift().getPathsDump();
     assertNotNull(pathDump);
     Map<Integer, TPathEntry> nodeMap = pathDump.getNodeMap();
-    assertTrue(nodeMap.size() > 0);
+    int nodeCount = prefixSize + dbCount + (dbCount * tableCount) +
+            (dbCount * tableCount * partitionCount);
+    assertTrue(nodeMap.size()  == nodeCount);
     assertEquals(notificationID, savedNotificationID);
   }
 


Mime
View raw message