You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by ak...@apache.org on 2017/06/03 01:18:44 UTC
sentry git commit: SENTRY-1743: Two SentryStore instances are one too
many (Alex Kolbasov, reviewed by: Sergio Pena and Vamsee Yarlagadda)
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 <ak...@cloudera.com>
Authored: Fri Jun 2 18:18:29 2017 -0700
Committer: Alexander Kolbasov <ak...@cloudera.com>
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");