Repository: sentry Updated Branches: refs/heads/master 2c5679006 -> dc7f2cf53
SENTRY-1743: Two SentryStore instances are one too many (Alex Kolbasov, reviewed by: Sergio Pena and Vamsee Yarlagadda) Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/dc7f2cf5 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/dc7f2cf5 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/dc7f2cf5 Branch: refs/heads/master Commit: dc7f2cf5373ca560df9ee3d532dc71dfc21659b8 Parents: 2c56790 Author: Alexander Kolbasov <[email protected]> Authored: Fri Jun 2 18:18:29 2017 -0700 Committer: Alexander Kolbasov <[email protected]> Committed: Fri Jun 2 18:18:29 2017 -0700 ---------------------------------------------------------------------- .../service/persistent/DelegateSentryStore.java | 4 +- .../db/service/persistent/SentryStore.java | 57 ++++++++++++++++++-- .../db/service/thrift/SentryAdminServlet.java | 2 +- .../thrift/SentryPolicyStoreProcessor.java | 6 +-- .../db/service/persistent/TestSentryStore.java | 2 +- .../persistent/TestSentryStoreImportExport.java | 2 +- .../service/persistent/TestSentryVersion.java | 2 +- .../e2e/sqoop/AbstractSqoopSentryTestBase.java | 2 + 8 files changed, 63 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/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 b130cc7..1cc5990 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 @@ -66,7 +66,7 @@ public class DelegateSentryStore implements SentryStoreLayer { this.privilegeOperator = new PrivilegeOperatePersistence(conf); this.conf = conf; //delegated old sentryStore - this.delegate = new SentryStore(conf); + this.delegate = SentryStore.getInstance(conf); adminGroups = ImmutableSet.copyOf(toTrimmed(Sets.newHashSet(conf.getStrings( ServerConfig.ADMIN_GROUPS, new String[]{})))); } @@ -389,7 +389,7 @@ public class DelegateSentryStore implements SentryStoreLayer { @Override public void close() { - delegate.stop(); + delegate.close(); } private Set<TSentryGroup> toTSentryGroups(Set<String> groups) { http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/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 2ca70e9..9022818 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 @@ -27,11 +27,13 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Properties; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import javax.jdo.FetchGroup; import javax.jdo.JDODataStoreException; @@ -83,7 +85,10 @@ import com.google.common.collect.Sets; * such as role and group names will be normalized to lowercase * in addition to starting and ending whitespace. */ -public class SentryStore { +public final class SentryStore { + // SentryStore is a singleton, except for testing purposes + private static final AtomicReference<SentryStore> instance = new AtomicReference<>(); + private static final Logger LOGGER = LoggerFactory .getLogger(SentryStore.class); @@ -104,7 +109,7 @@ public class SentryStore { // 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, + private static final Set<String> ALL_ACTIONS = ImmutableSet.of(AccessConstants.ALL, AccessConstants.SELECT, AccessConstants.INSERT, AccessConstants.ALTER, AccessConstants.CREATE, AccessConstants.DROP, AccessConstants.INDEX, AccessConstants.LOCK); @@ -112,14 +117,56 @@ public class SentryStore { // Now partial revoke just support action with SELECT,INSERT and ALL. // e.g. If we REVOKE SELECT from a privilege with action ALL, it will leads to INSERT // Otherwise, if we revoke other privilege(e.g. ALTER,DROP...), we will remove it from a role directly. - private static final Set<String> PARTIAL_REVOKE_ACTIONS = Sets.newHashSet(AccessConstants.ALL, + private static final Set<String> PARTIAL_REVOKE_ACTIONS = ImmutableSet.of(AccessConstants.ALL, AccessConstants.ACTION_ALL.toLowerCase(), AccessConstants.SELECT, AccessConstants.INSERT); private final PersistenceManagerFactory pmf; private Configuration conf; private final TransactionManager tm; - public SentryStore(Configuration conf) throws Exception { + /** + * return a singleton instance of the SentryStore + * @param conf Configuration object. Only used for the initial construction, + * ignored for all subsequent calls. + * @return instance of the SentryStore object. + * @throws Exception if something goes wrong during SentryStore creation + */ + public static SentryStore getInstance(Configuration conf) throws Exception { + // If there is an instance already, return it + SentryStore store = instance.get(); + if (store != null) { + LOGGER.debug("Using cached SentryStore store"); + return store; + } + LOGGER.debug("creating new SentryStore"); + // Create a new one + store = new SentryStore(conf); + // Save the new instance. In case someone beat us in the race, ignore the new instance + boolean ok = instance.compareAndSet(null, store); + if (ok) { + // Successfully saved, return the new value + return store; + } + // Someone already installed a new store, use it. + store.close(); + return instance.get(); + } + + /** + * Reset the static instance of SentryStore. This is only used by tests when they want to start + * a new SentryStore with a different configuration. + */ + public static void resetSentryStore() { + SentryStore store = instance.get(); + instance.set(null); + // Close old instance + if (store != null) { + store.close(); + } + } + + @VisibleForTesting + protected SentryStore(Configuration conf) throws Exception { this.conf = conf; Properties prop = new Properties(); prop.putAll(ServerConfig.SENTRY_STORE_DEFAULTS); @@ -217,7 +264,7 @@ public class SentryStore { } } - public synchronized void stop() { + public synchronized void close() { if (pmf != null) { pmf.close(); } http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java index 8a8bbd3..07372f3 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java @@ -114,7 +114,7 @@ public class SentryAdminServlet extends HttpServlet { Writer out = response.getWriter(); try { - SentryStore sentrystore = new SentryStore(conf); + SentryStore sentrystore = SentryStore.getInstance(conf); Map<String, Set<TSentryPrivilege>> roleMap = new HashMap<>(); Set<String> roleSet = sentrystore.getAllRoleNames(); for (String roleName: roleSet) { http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java index e9d9eec..b07fc59 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java @@ -99,12 +99,12 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { if (conf.getBoolean(ServerConfig.SENTRY_HA_ENABLED, ServerConfig.SENTRY_HA_ENABLED_DEFAULT)) { haContext = HAContext.getHAServerContext(conf); - sentryStore = new SentryStore(conf); + sentryStore = SentryStore.getInstance(conf); ServiceRegister reg = new ServiceRegister(haContext); reg.regService(conf.get(ServerConfig.RPC_ADDRESS), conf.getInt(ServerConfig.RPC_PORT,ServerConfig.RPC_PORT_DEFAULT)); } else { - sentryStore = new SentryStore(conf); + sentryStore = SentryStore.getInstance(conf); } isReady = true; adminGroups = ImmutableSet.copyOf(toTrimedLower(Sets.newHashSet(conf.getStrings( @@ -137,7 +137,7 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface { public void stop() { if (isReady) { - sentryStore.stop(); + sentryStore.close(); } if (haContext != null) { try { http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/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 3327e68..23b139c 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 @@ -110,7 +110,7 @@ public class TestSentryStore extends org.junit.Assert { @AfterClass public static void teardown() { if (sentryStore != null) { - sentryStore.stop(); + sentryStore.close(); } if (dataDir != null) { FileUtils.deleteQuietly(dataDir); http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java index d1a88b0..7706ee4 100644 --- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java +++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java @@ -130,7 +130,7 @@ public class TestSentryStoreImportExport { @AfterClass public static void teardown() { if (sentryStore != null) { - sentryStore.stop(); + sentryStore.close(); } if (dataDir != null) { FileUtils.deleteQuietly(dataDir); http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java index 103dbb6..85fd240 100644 --- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java +++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java @@ -54,7 +54,7 @@ public class TestSentryVersion { public void testVerifySentryVersionCheck() throws Exception { conf.set(ServerConfig.SENTRY_VERIFY_SCHEM_VERSION, "false"); SentryStore sentryStore = new SentryStore(conf); - sentryStore.stop(); + sentryStore.close(); conf.set(ServerConfig.SENTRY_VERIFY_SCHEM_VERSION, "true"); } http://git-wip-us.apache.org/repos/asf/sentry/blob/dc7f2cf5/sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java b/sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java index 67de5ac..b89fbd3 100644 --- a/sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java +++ b/sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java @@ -40,6 +40,7 @@ import org.apache.sentry.provider.db.generic.service.thrift.SentryGenericService import org.apache.sentry.provider.db.generic.service.thrift.SentryGenericServiceClientFactory; import org.apache.sentry.provider.db.generic.service.thrift.TAuthorizable; import org.apache.sentry.provider.db.generic.service.thrift.TSentryPrivilege; +import org.apache.sentry.provider.db.service.persistent.SentryStore; import org.apache.sentry.provider.file.LocalGroupResourceAuthorizationProvider; import org.apache.sentry.core.common.utils.PolicyFile; import org.apache.sentry.service.thrift.SentryService; @@ -114,6 +115,7 @@ public class AbstractSqoopSentryTestBase { } public static void setupConf() throws Exception { + SentryStore.resetSentryStore(); baseDir = createTempDir(); sqoopDir = new File(baseDir, "sqoop"); dbDir = new File(baseDir, "sentry_policy_db");
