EdColeman commented on code in PR #2569:
URL: https://github.com/apache/accumulo/pull/2569#discussion_r850681444
##########
server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java:
##########
@@ -19,134 +19,78 @@
package org.apache.accumulo.server.conf;
import java.util.Map;
-import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Predicate;
-import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.clientImpl.Namespace;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.data.NamespaceId;
-import org.apache.accumulo.fate.zookeeper.ZooCache;
-import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
import org.apache.accumulo.server.ServerContext;
-import org.apache.accumulo.server.conf.ZooCachePropertyAccessor.PropCacheKey;
+import org.apache.accumulo.server.conf.store.PropCacheKey;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-public class NamespaceConfiguration extends AccumuloConfiguration {
+public class NamespaceConfiguration extends ZooBasedConfiguration {
- private static final Map<PropCacheKey,ZooCache> propCaches = new
java.util.HashMap<>();
-
- private final AccumuloConfiguration parent;
- private final AtomicReference<ZooCachePropertyAccessor> propCacheAccessor =
- new AtomicReference<>();
- protected NamespaceId namespaceId = null;
+ private static final Logger log =
LoggerFactory.getLogger(NamespaceConfiguration.class);
protected ServerContext context;
- private ZooCacheFactory zcf = new ZooCacheFactory();
- private final String path;
public NamespaceConfiguration(NamespaceId namespaceId, ServerContext context,
AccumuloConfiguration parent) {
- this.context = context;
- this.parent = parent;
- this.namespaceId = namespaceId;
- this.path = context.getZooKeeperRoot() + Constants.ZNAMESPACES + "/" +
namespaceId
- + Constants.ZNAMESPACE_CONF;
+ super(log, context, PropCacheKey.forNamespace(context, namespaceId),
parent);
}
- /**
- * Gets the parent configuration of this configuration.
- *
- * @return parent configuration
- */
- public AccumuloConfiguration getParentConfiguration() {
- return parent;
- }
+ @Override
+ public String get(Property property) {
- void setZooCacheFactory(ZooCacheFactory zcf) {
- this.zcf = zcf;
- }
+ String key = property.getKey();
- private ZooCache getZooCache() {
- synchronized (propCaches) {
- PropCacheKey key = new PropCacheKey(context.getInstanceID(),
namespaceId.canonical());
- return propCaches.computeIfAbsent(key,
- k -> zcf.getZooCache(context.getZooKeepers(),
context.getZooKeepersSessionTimeOut()));
+ var namespaceId = getPropCacheKey().getNamespaceId();
+ if (namespaceId != null && namespaceId.equals(Namespace.ACCUMULO.id())
+ && isIteratorOrConstraint(key)) {
+ // ignore iterators from parent if system namespace
+ return null;
}
- }
-
- private ZooCachePropertyAccessor getPropCacheAccessor() {
- // updateAndGet below always calls compare and set, so avoid if not null
- ZooCachePropertyAccessor zcpa = propCacheAccessor.get();
- if (zcpa != null)
- return zcpa;
-
- return propCacheAccessor
- .updateAndGet(pca -> pca == null ? new
ZooCachePropertyAccessor(getZooCache()) : pca);
- }
-
- private String getPath() {
- return path;
- }
-
- @Override
- public boolean isPropertySet(Property prop, boolean cacheAndWatch) {
- if (!cacheAndWatch)
- throw new UnsupportedOperationException(
- "Namespace configuration only supports checking if a property is set
in cache.");
-
- if (getPropCacheAccessor().isPropertySet(prop, getPath()))
- return true;
- return parent.isPropertySet(prop, cacheAndWatch);
- }
+ Map<String,String> theseProps = getSnapshot();
+ String value = theseProps.get(key);
- @Override
- public String get(Property property) {
- String key = property.getKey();
- AccumuloConfiguration getParent;
- if (namespaceId.equals(Namespace.ACCUMULO.id()) &&
isIteratorOrConstraint(key)) {
- // ignore iterators from parent if system namespace
- getParent = null;
- } else {
- getParent = parent;
+ if (value != null) {
+ return value;
}
- return getPropCacheAccessor().get(property, getPath(), getParent);
+
+ return getParent().get(key);
Review Comment:
One issue is that Namespaceconfig and TableConfig are not the only classes
to extend AccumuloConfiguration - others like ConfigurationCopy do too - and
things using those are expecting a null at the top of the hierarchy. I believe
that anything that extends ZooBasedConfiguration can rely on the parent being
non-null because of the way the hierarchy is constructed - a table config will
always have a namespace parent, a namespace will always have a system parent,
and the system will always have a default - but past that, that may not be true
for an Accumulo configuration in general.
If there are specific, unnecessary checks in this PR, that could be removed.
I'd rather defer generalizing the whole null / non-null parent for the
Accumulo configuration to later. Some things may not be possible until a 3.0
(think ConfigurationCopy may fall under that).
The original code returned null as a parent at the top of the hierarchy. I
tried to maintain that behavior and changes to that behavior can be done
outside of this PR - unless I've made a logic error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
