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 <[email protected]> Authored: Fri Dec 14 09:47:28 2018 -0600 Committer: Kalyan Kumar Kalvagadda <[email protected]> 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); }
