This is an automated email from the ASF dual-hosted git repository. ctubbsii pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/main by this push: new ba244f6515 Use memoized suppliers to lazy load resources (#2658) ba244f6515 is described below commit ba244f6515be6d62cfdcfae86116beeb79e19960 Author: Christopher Tubbs <ctubb...@apache.org> AuthorDate: Thu Apr 28 13:47:21 2022 -0400 Use memoized suppliers to lazy load resources (#2658) * Remove the need to synchronize on reads for lazily initialized singleton resources, particularly in the configuration and context utilities, using Suppliers.memoize * Apply to DefaultConfiguration.getInstance() to avoid unnecessary object creation whenever that is called * Make all ServerContext fields final, and use memoize to lazily load anything that was previously checking if it was set in a synchronized getter method * Use computeIfAbsent in ServerConfigurationFactory, and make its caches of configuration objects non-static, so they only live as long as the ServerContext that created it lives; this, along with using ConcurrentHashMaps, dramatically simplifies this code * Reduce ServerContext reliance on ServerConfigurationFactory when it already has the information (notably, it has the instance of SiteConfiguration it used to construct the ServerConfigurationFactory) * Add a builder option for SiteConfiguration.empty() for use with tests * Update related tests, and make ServerContextTest.testCanRun more robust --- .../accumulo/core/conf/DefaultConfiguration.java | 9 +- .../accumulo/core/conf/SiteConfiguration.java | 10 +- .../accumulo/core/conf/SiteConfigurationTest.java | 8 +- ...tRegexTableLoadBalancerReconfigurationTest.java | 2 +- .../balancer/HostRegexTableLoadBalancerTest.java | 2 +- .../org/apache/accumulo/server/ServerContext.java | 122 ++++++---------- .../server/conf/ServerConfigurationFactory.java | 155 ++++++--------------- .../apache/accumulo/server/ServerContextTest.java | 58 +++++--- .../conf/ServerConfigurationFactoryTest.java | 30 ++-- .../BaseHostRegexTableLoadBalancerTest.java | 2 +- .../server/security/SystemCredentialsTest.java | 2 +- .../coordinator/CompactionCoordinator.java | 1 - .../org/apache/accumulo/compactor/Compactor.java | 1 - .../accumulo/gc/SimpleGarbageCollectorTest.java | 2 +- .../java/org/apache/accumulo/manager/Manager.java | 3 - .../accumulo/manager/upgrade/Upgrader9to10.java | 2 - .../org/apache/accumulo/tserver/TabletServer.java | 5 - 17 files changed, 159 insertions(+), 255 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java index fe08e5a3b3..7ba8026f62 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java @@ -18,9 +18,12 @@ */ package org.apache.accumulo.core.conf; +import static com.google.common.base.Suppliers.memoize; + import java.util.Arrays; import java.util.Map; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; /** @@ -29,7 +32,9 @@ import java.util.stream.Collectors; */ public class DefaultConfiguration extends AccumuloConfiguration { - private static final Map<String,String> resolvedProps = + private static final Supplier<DefaultConfiguration> instance = memoize(DefaultConfiguration::new); + + private final Map<String,String> resolvedProps = Arrays.stream(Property.values()).filter(p -> p.getType() != PropertyType.PREFIX) .collect(Collectors.toMap(Property::getKey, Property::getDefaultValue)); @@ -41,7 +46,7 @@ public class DefaultConfiguration extends AccumuloConfiguration { * @return default configuration */ public static DefaultConfiguration getInstance() { - return new DefaultConfiguration(); + return instance.get(); } @Override diff --git a/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java index 63c5c28334..e1c6ed3ea7 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java @@ -77,8 +77,7 @@ public class SiteConfiguration extends AccumuloConfiguration { // visible to package-private for testing only Builder() {} - // exists for testing only - OverridesOption noFile() { + private OverridesOption noFile() { return this; } @@ -196,6 +195,13 @@ public class SiteConfiguration extends AccumuloConfiguration { return new SiteConfiguration.Builder().fromFile(propertiesFileLocation); } + /** + * Build a SiteConfiguration that is initially empty with the option to override. + */ + public static SiteConfiguration.OverridesOption empty() { + return new SiteConfiguration.Builder().noFile(); + } + /** * Build a SiteConfiguration from the environmental configuration and no overrides. */ diff --git a/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java b/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java index 3d30520c31..39e1cc1237 100644 --- a/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java +++ b/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java @@ -46,7 +46,7 @@ public class SiteConfigurationTest { var overrides = Map.of(Property.GENERAL_SECURITY_CREDENTIAL_PROVIDER_PATHS.getKey(), credProvPath); - var config = new SiteConfiguration.Builder().noFile().withOverrides(overrides).build(); + var config = SiteConfiguration.empty().withOverrides(overrides).build(); assertEquals("mysecret", config.get(Property.INSTANCE_SECRET)); assertNull(config.get("ignored.property")); @@ -56,7 +56,7 @@ public class SiteConfigurationTest { @Test public void testDefault() { - var conf = SiteConfiguration.auto(); + var conf = SiteConfiguration.empty().build(); assertEquals("localhost:2181", conf.get(Property.INSTANCE_ZK_HOST)); assertEquals("DEFAULT", conf.get(Property.INSTANCE_SECRET)); assertEquals("", conf.get(Property.INSTANCE_VOLUMES)); @@ -84,10 +84,10 @@ public class SiteConfigurationTest { @Test public void testConfigOverrides() { - var conf = SiteConfiguration.auto(); + var conf = SiteConfiguration.empty().build(); assertEquals("localhost:2181", conf.get(Property.INSTANCE_ZK_HOST)); - conf = new SiteConfiguration.Builder().noFile() + conf = SiteConfiguration.empty() .withOverrides(Map.of(Property.INSTANCE_ZK_HOST.getKey(), "myhost:2181")).build(); assertEquals("myhost:2181", conf.get(Property.INSTANCE_ZK_HOST)); diff --git a/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerReconfigurationTest.java b/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerReconfigurationTest.java index 3878a41a3f..8b47f0e9c6 100644 --- a/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerReconfigurationTest.java +++ b/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerReconfigurationTest.java @@ -63,7 +63,7 @@ public class HostRegexTableLoadBalancerReconfigurationTest tables.put(BAR.getTableName(), BAR.getId()); tables.put(BAZ.getTableName(), BAZ.getId()); - ConfigurationCopy config = new ConfigurationCopy(SiteConfiguration.auto()); + ConfigurationCopy config = new ConfigurationCopy(SiteConfiguration.empty().build()); DEFAULT_TABLE_PROPERTIES.forEach(config::set); ConfigurationImpl configImpl = new ConfigurationImpl(config); BalancerEnvironment environment = createMock(BalancerEnvironment.class); diff --git a/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerTest.java b/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerTest.java index 9a96af1a8e..1147c220ae 100644 --- a/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerTest.java +++ b/core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerTest.java @@ -65,7 +65,7 @@ public class HostRegexTableLoadBalancerTest extends BaseHostRegexTableLoadBalanc tables.put(BAR.getTableName(), BAR.getId()); tables.put(BAZ.getTableName(), BAZ.getId()); - ConfigurationCopy config = new ConfigurationCopy(SiteConfiguration.auto()); + ConfigurationCopy config = new ConfigurationCopy(SiteConfiguration.empty().build()); tableProperties.forEach(config::set); ConfigurationImpl configImpl = new ConfigurationImpl(config); BalancerEnvironment environment = createMock(BalancerEnvironment.class); diff --git a/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java b/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java index 398f73a01a..0ed173195a 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java +++ b/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java @@ -19,6 +19,7 @@ package org.apache.accumulo.server; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Suppliers.memoize; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.MINUTES; import static java.util.concurrent.TimeUnit.SECONDS; @@ -37,6 +38,7 @@ import java.util.TreeMap; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.clientImpl.ClientContext; @@ -91,15 +93,15 @@ public class ServerContext extends ClientContext { private final ZooReaderWriter zooReaderWriter; private final ServerDirs serverDirs; - private TableManager tableManager; - private UniqueNameAllocator nameAllocator; - private ServerConfigurationFactory serverConfFactory = null; - private DefaultConfiguration defaultConfig = null; - private AccumuloConfiguration systemConfig = null; - private AuthenticationTokenSecretManager secretManager; - private CryptoService cryptoService = null; - private ScheduledThreadPoolExecutor sharedScheduledThreadPool = null; - private AuditedSecurityOperation securityOperation = null; + // lazily loaded resources, only loaded when needed + private final Supplier<TableManager> tableManager; + private final Supplier<UniqueNameAllocator> nameAllocator; + private final Supplier<ServerConfigurationFactory> serverConfFactory; + private final Supplier<AccumuloConfiguration> systemConfig; + private final Supplier<AuthenticationTokenSecretManager> secretManager; + private final Supplier<CryptoService> cryptoService; + private final Supplier<ScheduledThreadPoolExecutor> sharedScheduledThreadPool; + private final Supplier<AuditedSecurityOperation> securityOperation; public ServerContext(SiteConfiguration siteConfig) { this(new ServerInfo(siteConfig)); @@ -110,6 +112,23 @@ public class ServerContext extends ClientContext { this.info = info; zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration()); serverDirs = info.getServerDirs(); + + tableManager = memoize(() -> new TableManager(this)); + nameAllocator = memoize(() -> new UniqueNameAllocator(this)); + serverConfFactory = memoize(() -> new ServerConfigurationFactory(this, getSiteConfiguration())); + // system configuration uses its own instance of ZooCache + // this could be useful to keep its update counter independent + systemConfig = memoize(() -> new ZooConfiguration(this, new ZooCache(getZooReader(), null), + getSiteConfiguration())); + secretManager = memoize(() -> new AuthenticationTokenSecretManager(getInstanceID(), + getConfiguration().getTimeInMillis(Property.GENERAL_DELEGATION_TOKEN_LIFETIME))); + cryptoService = memoize( + () -> CryptoServiceFactory.newInstance(getConfiguration(), ClassloaderType.ACCUMULO)); + sharedScheduledThreadPool = memoize(() -> ThreadPools.getServerThreadPools() + .createGeneralScheduledExecutorService(getConfiguration())); + securityOperation = + memoize(() -> new AuditedSecurityOperation(this, SecurityOperation.getAuthorizor(this), + SecurityOperation.getAuthenticator(this), SecurityOperation.getPermHandler(this))); } /** @@ -134,54 +153,25 @@ public class ServerContext extends ClientContext { return info.getInstanceID(); } - /** - * Should only be called by the Tablet server - */ - public synchronized void setupCrypto() throws CryptoService.CryptoException { - if (cryptoService != null) { - throw new CryptoService.CryptoException("Crypto Service " + cryptoService.getClass().getName() - + " already exists and cannot be setup again"); - } - - AccumuloConfiguration acuConf = getConfiguration(); - cryptoService = CryptoServiceFactory.newInstance(acuConf, ClassloaderType.ACCUMULO); - } - public SiteConfiguration getSiteConfiguration() { return info.getSiteConfiguration(); } - public synchronized ServerConfigurationFactory getServerConfFactory() { - if (serverConfFactory == null) { - serverConfFactory = new ServerConfigurationFactory(this, info.getSiteConfiguration()); - } - return serverConfFactory; - } - @Override public AccumuloConfiguration getConfiguration() { - if (systemConfig == null) { - // system configuration uses its own instance of ZooCache - // this could be useful to keep its update counter independent - ZooCache propCache = new ZooCache(getZooReader(), null); - systemConfig = new ZooConfiguration(this, propCache, getSiteConfiguration()); - } - return systemConfig; + return systemConfig.get(); } public TableConfiguration getTableConfiguration(TableId id) { - return getServerConfFactory().getTableConfiguration(id); + return serverConfFactory.get().getTableConfiguration(id); } public NamespaceConfiguration getNamespaceConfiguration(NamespaceId namespaceId) { - return getServerConfFactory().getNamespaceConfiguration(namespaceId); + return serverConfFactory.get().getNamespaceConfiguration(namespaceId); } public DefaultConfiguration getDefaultConfiguration() { - if (defaultConfig == null) { - defaultConfig = DefaultConfiguration.getInstance(); - } - return defaultConfig; + return DefaultConfiguration.getInstance(); } public ServerDirs getServerDirs() { @@ -194,7 +184,7 @@ public class ServerContext extends ClientContext { */ // Should be private, but package-protected so EasyMock will work void enforceKerberosLogin() { - final AccumuloConfiguration conf = getServerConfFactory().getSiteConfiguration(); + final AccumuloConfiguration conf = getSiteConfiguration(); // Unwrap _HOST into the FQDN to make the kerberos principal we'll compare against final String kerberosPrincipal = SecurityUtil.getServerPrincipal(conf.get(Property.GENERAL_KERBEROS_PRINCIPAL)); @@ -234,11 +224,11 @@ public class ServerContext extends ClientContext { @Override public SaslServerConnectionParams getSaslParams() { - AccumuloConfiguration conf = getServerConfFactory().getSiteConfiguration(); + AccumuloConfiguration conf = getSiteConfiguration(); if (!conf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) { return null; } - return new SaslServerConnectionParams(conf, getCredentials().getToken(), secretManager); + return new SaslServerConnectionParams(conf, getCredentials().getToken(), getSecretManager()); } /** @@ -269,33 +259,20 @@ public class ServerContext extends ClientContext { } } - public void setSecretManager(AuthenticationTokenSecretManager secretManager) { - this.secretManager = secretManager; - } - public AuthenticationTokenSecretManager getSecretManager() { - return secretManager; + return secretManager.get(); } - public synchronized TableManager getTableManager() { - if (tableManager == null) { - tableManager = new TableManager(this); - } - return tableManager; + public TableManager getTableManager() { + return tableManager.get(); } - public synchronized UniqueNameAllocator getUniqueNameAllocator() { - if (nameAllocator == null) { - nameAllocator = new UniqueNameAllocator(this); - } - return nameAllocator; + public UniqueNameAllocator getUniqueNameAllocator() { + return nameAllocator.get(); } public CryptoService getCryptoService() { - if (cryptoService == null) { - throw new CryptoService.CryptoException("Crypto service not initialized."); - } - return cryptoService; + return cryptoService.get(); } @Override @@ -453,15 +430,8 @@ public class ServerContext extends ClientContext { ThreadPools.watchNonCriticalScheduledTask(future); } - /** - * return a shared scheduled executor - */ - public synchronized ScheduledThreadPoolExecutor getScheduledExecutor() { - if (sharedScheduledThreadPool == null) { - sharedScheduledThreadPool = ThreadPools.getServerThreadPools() - .createGeneralScheduledExecutorService(getConfiguration()); - } - return sharedScheduledThreadPool; + public ScheduledThreadPoolExecutor getScheduledExecutor() { + return sharedScheduledThreadPool.get(); } @Override @@ -470,11 +440,7 @@ public class ServerContext extends ClientContext { } public AuditedSecurityOperation getSecurityOperation() { - if (securityOperation == null) { - securityOperation = new AuditedSecurityOperation(this, SecurityOperation.getAuthorizor(this), - SecurityOperation.getAuthenticator(this), SecurityOperation.getPermHandler(this)); - } - return securityOperation; + return securityOperation.get(); } } diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java b/server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java index 7b32e747b7..d968af064e 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java +++ b/server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java @@ -18,67 +18,50 @@ */ package org.apache.accumulo.server.conf; -import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; import org.apache.accumulo.core.client.TableNotFoundException; import org.apache.accumulo.core.conf.AccumuloConfiguration; import org.apache.accumulo.core.conf.ConfigCheckUtil; import org.apache.accumulo.core.conf.DefaultConfiguration; import org.apache.accumulo.core.conf.SiteConfiguration; -import org.apache.accumulo.core.data.InstanceId; import org.apache.accumulo.core.data.NamespaceId; import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.fate.zookeeper.ZooCache; import org.apache.accumulo.fate.zookeeper.ZooCacheFactory; import org.apache.accumulo.server.ServerContext; +import com.google.common.base.Suppliers; + /** * A factor for configurations used by a server process. Instance of this class are thread-safe. */ public class ServerConfigurationFactory extends ServerConfiguration { - private static final Map<InstanceId,Map<TableId,TableConfiguration>> tableConfigs = - new HashMap<>(1); - private static final Map<InstanceId,Map<NamespaceId,NamespaceConfiguration>> namespaceConfigs = - new HashMap<>(1); - private static final Map<InstanceId,Map<TableId,NamespaceConfiguration>> tableParentConfigs = - new HashMap<>(1); + private final Map<TableId,NamespaceConfiguration> tableParentConfigs = new ConcurrentHashMap<>(); + private final Map<TableId,TableConfiguration> tableConfigs = new ConcurrentHashMap<>(); + private final Map<NamespaceId,NamespaceConfiguration> namespaceConfigs = + new ConcurrentHashMap<>(); - private static void addInstanceToCaches(InstanceId iid) { - synchronized (tableConfigs) { - tableConfigs.computeIfAbsent(iid, k -> new HashMap<>()); - } - synchronized (namespaceConfigs) { - namespaceConfigs.computeIfAbsent(iid, k -> new HashMap<>()); - } - synchronized (tableParentConfigs) { - tableParentConfigs.computeIfAbsent(iid, k -> new HashMap<>()); - } - } - - static void clearCachedConfigurations() { - synchronized (tableConfigs) { - tableConfigs.clear(); - } - synchronized (namespaceConfigs) { - namespaceConfigs.clear(); - } - synchronized (tableParentConfigs) { - tableParentConfigs.clear(); - } - } + private final Supplier<ZooConfiguration> systemConfig; private final ServerContext context; private final SiteConfiguration siteConfig; - private final InstanceId instanceID; private ZooCacheFactory zcf = new ZooCacheFactory(); public ServerConfigurationFactory(ServerContext context, SiteConfiguration siteConfig) { this.context = context; this.siteConfig = siteConfig; - instanceID = context.getInstanceID(); - addInstanceToCaches(instanceID); + systemConfig = Suppliers.memoize(() -> { + // Force the creation of a new ZooCache instead of using a shared one. + // This is done so that the ZooCache will update less often, causing the + // configuration update count to increment more slowly. + ZooCache propCache = + zcf.getNewZooCache(context.getZooKeepers(), context.getZooKeepersSessionTimeOut()); + return new ZooConfiguration(context, propCache, siteConfig); + }); } public ServerContext getServerContext() { @@ -89,109 +72,51 @@ public class ServerConfigurationFactory extends ServerConfiguration { this.zcf = zcf; } - private DefaultConfiguration defaultConfig = null; - private AccumuloConfiguration systemConfig = null; - public SiteConfiguration getSiteConfiguration() { return siteConfig; } - public synchronized DefaultConfiguration getDefaultConfiguration() { - if (defaultConfig == null) { - defaultConfig = DefaultConfiguration.getInstance(); - } - return defaultConfig; + public DefaultConfiguration getDefaultConfiguration() { + return DefaultConfiguration.getInstance(); } @Override - public synchronized AccumuloConfiguration getSystemConfiguration() { - if (systemConfig == null) { - // Force the creation of a new ZooCache instead of using a shared one. - // This is done so that the ZooCache will update less often, causing the - // configuration update count to increment more slowly. - ZooCache propCache = - zcf.getNewZooCache(context.getZooKeepers(), context.getZooKeepersSessionTimeOut()); - systemConfig = new ZooConfiguration(context, propCache, getSiteConfiguration()); - } - return systemConfig; + public AccumuloConfiguration getSystemConfiguration() { + return systemConfig.get(); } @Override public TableConfiguration getTableConfiguration(TableId tableId) { - TableConfiguration conf; - synchronized (tableConfigs) { - conf = tableConfigs.get(instanceID).get(tableId); - } - - // Can't hold the lock during the construction and validation of the config, - // which would result in creating multiple objects for the same id. - // - // ACCUMULO-3859 We _cannot_ allow multiple instances to be created for a table. If the - // TableConfiguration - // instance a Tablet holds is not the same as the one cached here, any ConfigurationObservers - // that - // Tablet sets will never see updates from ZooKeeper which means that things like constraints - // and - // default visibility labels will never be updated in a Tablet until it is reloaded. - if (conf == null && context.tableNodeExists(tableId)) { - conf = new TableConfiguration(context, tableId, getNamespaceConfigurationForTable(tableId)); - ConfigCheckUtil.validate(conf); - synchronized (tableConfigs) { - Map<TableId,TableConfiguration> configs = tableConfigs.get(instanceID); - TableConfiguration existingConf = configs.get(tableId); - if (existingConf == null) { - // Configuration doesn't exist yet - configs.put(tableId, conf); - } else { - // Someone beat us to the punch, reuse their instance instead of replacing it - conf = existingConf; - } + return tableConfigs.computeIfAbsent(tableId, key -> { + if (context.tableNodeExists(tableId)) { + var conf = + new TableConfiguration(context, tableId, getNamespaceConfigurationForTable(tableId)); + ConfigCheckUtil.validate(conf); + return conf; } - } - return conf; + return null; + }); } public NamespaceConfiguration getNamespaceConfigurationForTable(TableId tableId) { - NamespaceConfiguration conf; - synchronized (tableParentConfigs) { - conf = tableParentConfigs.get(instanceID).get(tableId); - } - // can't hold the lock during the construction and validation of the config, - // which may result in creating multiple objects for the same id, but that's ok. - if (conf == null) { - NamespaceId namespaceId; - try { - namespaceId = context.getNamespaceId(tableId); - } catch (TableNotFoundException e) { - throw new RuntimeException(e); - } - conf = new NamespaceConfiguration(namespaceId, context, getSystemConfiguration()); - ConfigCheckUtil.validate(conf); - synchronized (tableParentConfigs) { - tableParentConfigs.get(instanceID).put(tableId, conf); - } + NamespaceId namespaceId; + try { + namespaceId = context.getNamespaceId(tableId); + } catch (TableNotFoundException e) { + throw new RuntimeException(e); } - return conf; + return tableParentConfigs.computeIfAbsent(tableId, + key -> getNamespaceConfiguration(namespaceId)); } @Override public NamespaceConfiguration getNamespaceConfiguration(NamespaceId namespaceId) { - NamespaceConfiguration conf; - // can't hold the lock during the construction and validation of the config, - // which may result in creating multiple objects for the same id, but that's ok. - synchronized (namespaceConfigs) { - conf = namespaceConfigs.get(instanceID).get(namespaceId); - } - if (conf == null) { - // changed - include instance in constructor call - conf = new NamespaceConfiguration(namespaceId, context, getSystemConfiguration()); + return namespaceConfigs.computeIfAbsent(namespaceId, key -> { + var conf = new NamespaceConfiguration(namespaceId, context, getSystemConfiguration()); conf.setZooCacheFactory(zcf); ConfigCheckUtil.validate(conf); - synchronized (namespaceConfigs) { - namespaceConfigs.get(instanceID).put(namespaceId, conf); - } - } - return conf; + return conf; + }); } } diff --git a/server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java b/server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java index ab1912952a..e216ddae35 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java @@ -18,6 +18,14 @@ */ package org.apache.accumulo.server; +import static org.easymock.EasyMock.anyObject; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.createMockBuilder; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.getCurrentArguments; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -27,6 +35,8 @@ import java.io.DataInputStream; import java.io.DataOutputStream; import java.security.PrivilegedExceptionAction; import java.util.Properties; +import java.util.function.IntConsumer; +import java.util.stream.IntStream; import org.apache.accumulo.core.client.security.tokens.PasswordToken; import org.apache.accumulo.core.clientImpl.ClientConfConverter; @@ -42,7 +52,6 @@ import org.apache.accumulo.server.security.SystemCredentials.SystemToken; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.CommonConfigurationKeys; import org.apache.hadoop.security.UserGroupInformation; -import org.easymock.EasyMock; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -71,9 +80,9 @@ public class ServerContextTest { clientProps.setProperty(ClientProperty.SASL_ENABLED.getKey(), "true"); clientProps.setProperty(ClientProperty.SASL_KERBEROS_SERVER_PRIMARY.getKey(), "accumulo"); final AccumuloConfiguration conf = ClientConfConverter.toAccumuloConf(clientProps); - SiteConfiguration siteConfig = EasyMock.createMock(SiteConfiguration.class); + SiteConfiguration siteConfig = createMock(SiteConfiguration.class); - EasyMock.expect(siteConfig.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)).andReturn(true); + expect(siteConfig.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)).andReturn(true); // Deal with SystemToken being private PasswordToken pw = new PasswordToken("fake"); @@ -82,38 +91,39 @@ public class ServerContextTest { SystemToken token = new SystemToken(); token.readFields(new DataInputStream(new ByteArrayInputStream(baos.toByteArray()))); - ServerConfigurationFactory factory = EasyMock.createMock(ServerConfigurationFactory.class); - EasyMock.expect(factory.getSystemConfiguration()).andReturn(conf).anyTimes(); - EasyMock.expect(factory.getSiteConfiguration()).andReturn(siteConfig).anyTimes(); + ServerConfigurationFactory factory = createMock(ServerConfigurationFactory.class); + expect(factory.getSystemConfiguration()).andReturn(conf).anyTimes(); - ServerContext context = EasyMock.createMockBuilder(ServerContext.class) - .addMockedMethod("enforceKerberosLogin").addMockedMethod("getConfiguration") - .addMockedMethod("getServerConfFactory").addMockedMethod("getCredentials").createMock(); + ServerContext context = + createMockBuilder(ServerContext.class).addMockedMethod("enforceKerberosLogin") + .addMockedMethod("getConfiguration").addMockedMethod("getSiteConfiguration") + .addMockedMethod("getSecretManager").addMockedMethod("getCredentials").createMock(); context.enforceKerberosLogin(); - EasyMock.expectLastCall().anyTimes(); - EasyMock.expect(context.getConfiguration()).andReturn(conf).anyTimes(); - EasyMock.expect(context.getServerConfFactory()).andReturn(factory).anyTimes(); - EasyMock.expect(context.getCredentials()) + expectLastCall().anyTimes(); + expect(context.getSiteConfiguration()).andReturn(siteConfig).anyTimes(); + expect(context.getSecretManager()).andReturn(null).anyTimes(); + expect(context.getConfiguration()).andReturn(conf).anyTimes(); + expect(context.getCredentials()) .andReturn(new Credentials("accumulo/hostn...@fake.com", token)).once(); // Just make the SiteConfiguration delegate to our ClientConfiguration (by way of the // AccumuloConfiguration) // Presently, we only need get(Property) and iterator(). - EasyMock.expect(siteConfig.get(EasyMock.anyObject(Property.class))).andAnswer(() -> { - Object[] args = EasyMock.getCurrentArguments(); + expect(siteConfig.get(anyObject(Property.class))).andAnswer(() -> { + Object[] args = getCurrentArguments(); return conf.get((Property) args[0]); }).anyTimes(); - EasyMock.expect(siteConfig.iterator()).andAnswer(conf::iterator).anyTimes(); + expect(siteConfig.iterator()).andAnswer(conf::iterator).anyTimes(); - EasyMock.replay(factory, context, siteConfig); + replay(factory, context, siteConfig); assertEquals(ThriftServerType.SASL, context.getThriftServerType()); SaslServerConnectionParams saslParams = context.getSaslParams(); assertEquals(new SaslServerConnectionParams(conf, token), saslParams); assertEquals(username, saslParams.getPrincipal()); - EasyMock.verify(factory, context, siteConfig); + verify(factory, context, siteConfig); return null; }); @@ -121,8 +131,16 @@ public class ServerContextTest { @Test public void testCanRun() { - // ensure this fails with older versions - assertThrows(IllegalStateException.class, () -> ServerContext.ensureDataVersionCompatible(7)); + // ensure this fails with older versions; the oldest supported version is hard-coded here + // to ensure we don't unintentionally break upgrade support; changing this should be a conscious + // decision and this check will ensure we don't overlook it + final int oldestSupported = 8; + final int currentVersion = AccumuloDataVersion.get(); + IntConsumer shouldPass = v -> ServerContext.ensureDataVersionCompatible(v); + IntConsumer shouldFail = v -> assertThrows(IllegalStateException.class, + () -> ServerContext.ensureDataVersionCompatible(v)); + IntStream.rangeClosed(oldestSupported, currentVersion).forEach(shouldPass); + IntStream.of(oldestSupported - 1, currentVersion + 1).forEach(shouldFail); } } diff --git a/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java b/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java index 1657734d10..1a7ad40410 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java @@ -18,13 +18,12 @@ */ package org.apache.accumulo.server.conf; -import static java.nio.charset.StandardCharsets.UTF_8; import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.createMock; -import static org.easymock.EasyMock.endsWith; import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertSame; @@ -38,6 +37,7 @@ import org.apache.accumulo.fate.zookeeper.ZooCache; import org.apache.accumulo.fate.zookeeper.ZooCacheFactory; import org.apache.accumulo.server.MockServerContext; import org.apache.accumulo.server.ServerContext; +import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -51,7 +51,7 @@ public class ServerConfigurationFactoryTest { // use the same mock ZooCacheFactory and ZooCache for all tests private static ZooCacheFactory zcf; private static ZooCache zc; - private static SiteConfiguration siteConfig = SiteConfiguration.auto(); + private static SiteConfiguration siteConfig = SiteConfiguration.empty().build(); @BeforeAll public static void setUpClass() { @@ -62,47 +62,44 @@ public class ServerConfigurationFactoryTest { replay(zcf); expect(zc.getChildren(anyObject(String.class))).andReturn(null).anyTimes(); - // CheckServerConfig looks at timeout - expect(zc.get(endsWith("timeout"))).andReturn(("" + ZK_TIMEOUT + "ms").getBytes(UTF_8)); replay(zc); } + @AfterAll + public static void verifyClassMocks() { + verify(zcf, zc); + } + private ServerContext context; private ServerConfigurationFactory scf; @BeforeEach public void setUp() { context = MockServerContext.getWithZK(IID, ZK_HOST, ZK_TIMEOUT); - } - - @AfterEach - public void tearDown() { - ServerConfigurationFactory.clearCachedConfigurations(); - } - - private void ready() { replay(context); scf = new ServerConfigurationFactory(context, siteConfig); scf.setZooCacheFactory(zcf); } + @AfterEach + public void verifyMocks() { + verify(context); + } + @Test public void testGetDefaultConfiguration() { - ready(); DefaultConfiguration c = scf.getDefaultConfiguration(); assertNotNull(c); } @Test public void testGetSiteConfiguration() { - ready(); var c = scf.getSiteConfiguration(); assertNotNull(c); } @Test public void testGetConfiguration() { - ready(); AccumuloConfiguration c = scf.getSystemConfiguration(); assertNotNull(c); } @@ -111,7 +108,6 @@ public class ServerConfigurationFactoryTest { @Test public void testGetNamespaceConfiguration() { - ready(); NamespaceConfiguration c = scf.getNamespaceConfiguration(NSID); assertEquals(NSID, c.getNamespaceId()); assertSame(c, scf.getNamespaceConfiguration(NSID)); diff --git a/server/base/src/test/java/org/apache/accumulo/server/master/balancer/BaseHostRegexTableLoadBalancerTest.java b/server/base/src/test/java/org/apache/accumulo/server/master/balancer/BaseHostRegexTableLoadBalancerTest.java index 01ca53703e..cf767c54c5 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/master/balancer/BaseHostRegexTableLoadBalancerTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/master/balancer/BaseHostRegexTableLoadBalancerTest.java @@ -92,7 +92,7 @@ public abstract class BaseHostRegexTableLoadBalancerTest extends HostRegexTableL TestDefaultBalancer.class.getName()); } - private static SiteConfiguration siteConfg = SiteConfiguration.auto(); + private static SiteConfiguration siteConfg = SiteConfiguration.empty().build(); protected static class TestServerConfigurationFactory extends ServerConfigurationFactory { diff --git a/server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java b/server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java index 7a3ecc19ad..3b8f0b1a86 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java @@ -39,7 +39,7 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; public class SystemCredentialsTest { - private static SiteConfiguration siteConfig = SiteConfiguration.auto(); + private static SiteConfiguration siteConfig = SiteConfiguration.empty().build(); private InstanceId instanceId = InstanceId.of(UUID.nameUUIDFromBytes(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 0})); diff --git a/server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java b/server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java index 1dc1a01f85..87766f3bb1 100644 --- a/server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java +++ b/server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java @@ -157,7 +157,6 @@ public class CompactionCoordinator extends AbstractServer } protected void setupSecurity() { - getContext().setupCrypto(); security = getContext().getSecurityOperation(); } diff --git a/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java b/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java index 4646f1613d..4d7da8b79f 100644 --- a/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java +++ b/server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java @@ -191,7 +191,6 @@ public class Compactor extends AbstractServer implements MetricsProducer, Compac } protected void setupSecurity() { - getContext().setupCrypto(); security = getContext().getSecurityOperation(); } diff --git a/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java b/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java index 9c7541f639..337a816ee0 100644 --- a/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java +++ b/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java @@ -60,7 +60,7 @@ public class SimpleGarbageCollectorTest { private Credentials credentials; private SimpleGarbageCollector gc; private ConfigurationCopy systemConfig; - private static SiteConfiguration siteConfig = SiteConfiguration.auto(); + private static SiteConfiguration siteConfig = SiteConfiguration.empty().build(); @BeforeEach public void setUp() { diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java index da26715ff9..6bb41f0be4 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java @@ -130,7 +130,6 @@ import org.apache.accumulo.server.rpc.TServerUtils; import org.apache.accumulo.server.rpc.ThriftProcessorTypes; import org.apache.accumulo.server.security.SecurityOperation; import org.apache.accumulo.server.security.delegation.AuthenticationTokenKeyManager; -import org.apache.accumulo.server.security.delegation.AuthenticationTokenSecretManager; import org.apache.accumulo.server.security.delegation.ZooAuthenticationKeyDistributor; import org.apache.accumulo.server.tables.TableManager; import org.apache.accumulo.server.tables.TableObserver; @@ -386,9 +385,7 @@ public class Manager extends AbstractServer this.security = context.getSecurityOperation(); - // Create the secret manager (can generate and verify delegation tokens) final long tokenLifetime = aconf.getTimeInMillis(Property.GENERAL_DELEGATION_TOKEN_LIFETIME); - context.setSecretManager(new AuthenticationTokenSecretManager(getInstanceID(), tokenLifetime)); authenticationTokenKeyManager = null; keyDistributor = null; diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java b/server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java index 6cfed37ec3..20136e7fad 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java @@ -376,8 +376,6 @@ public class Upgrader9to10 implements Upgrader { MetadataTime computeRootTabletTime(ServerContext context, Collection<String> goodPaths) { try { - context.setupCrypto(); - long rtime = Long.MIN_VALUE; for (String good : goodPaths) { Path path = new Path(good); diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java index 9820f3e165..910c4e9303 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java @@ -124,7 +124,6 @@ import org.apache.accumulo.server.rpc.TServerUtils; import org.apache.accumulo.server.rpc.ThriftProcessorTypes; import org.apache.accumulo.server.security.SecurityOperation; import org.apache.accumulo.server.security.SecurityUtil; -import org.apache.accumulo.server.security.delegation.AuthenticationTokenSecretManager; import org.apache.accumulo.server.security.delegation.ZooAuthenticationKeyWatcher; import org.apache.accumulo.server.util.FileSystemMonitor; import org.apache.accumulo.server.util.ServerBulkImportStatus; @@ -247,7 +246,6 @@ public class TabletServer extends AbstractServer { protected TabletServer(ServerOpts opts, String[] args) { super("tserver", opts, args); context = super.getContext(); - context.setupCrypto(); this.managerLockCache = new ZooCache(context.getZooReader(), null); final AccumuloConfiguration aconf = getConfiguration(); log.info("Version " + Constants.VERSION); @@ -363,9 +361,6 @@ public class TabletServer extends AbstractServer { TabletLocator::clearLocators, jitter(), jitter(), TimeUnit.MILLISECONDS)); walMarker = new WalStateManager(context); - // Create the secret manager - context.setSecretManager(new AuthenticationTokenSecretManager(context.getInstanceID(), - aconf.getTimeInMillis(Property.GENERAL_DELEGATION_TOKEN_LIFETIME))); if (aconf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) { log.info("SASL is enabled, creating ZooKeeper watcher for AuthenticationKeys"); // Watcher to notice new AuthenticationKeys which enable delegation tokens