dlmarion commented on a change in pull request #2569:
URL: https://github.com/apache/accumulo/pull/2569#discussion_r828432480
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java
##########
@@ -106,12 +108,7 @@ public synchronized DefaultConfiguration
getDefaultConfiguration() {
@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());
+ systemConfig = new SystemConfiguration(log, context, sysPropCacheId,
getSiteConfiguration());
}
return systemConfig;
Review comment:
why not just return `context.getConfiguration()` ?
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/conf/ZooBasedConfiguration.java
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.conf;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Predicate;
+
+import org.apache.accumulo.core.conf.AccumuloConfiguration;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.conf.store.PropCacheId;
+import org.apache.accumulo.server.conf.store.PropChangeListener;
+import org.apache.accumulo.server.conf.store.PropStore;
+import org.apache.accumulo.server.conf.store.PropStoreException;
+import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.slf4j.Logger;
+
+/**
+ * Instances maintain a local cache of the AccumuloConfiguration hierarchy
that will be consistent
+ * with stored properties.
+ * <p>
+ * When calling getProperties - the local copy will be updated if ZooKeeper
changes have been
+ * received.
+ * <p>
+ * The getUpdateCount() provides an optimization for clients - the count can
be used to detect
+ * changes without reading the properties. When the update count changes, the
next getProperties
+ * call will update the local copy and the change count.
+ */
+public class ZooBasedConfiguration extends AccumuloConfiguration implements
PropChangeListener {
+
+ protected final Logger log;
+ private final AccumuloConfiguration parent;
+ private final PropCacheId propCacheId;
+ private final PropStore propStore;
+
+ private final AtomicReference<PropSnapshot> snapshotRef = new
AtomicReference<>(null);
+
+ public ZooBasedConfiguration(Logger log, ServerContext context, PropCacheId
propCacheId,
+ AccumuloConfiguration parent) {
+ this.log = requireNonNull(log, "a Logger must be supplied");
+ requireNonNull(context, "the context cannot be null");
+ this.propCacheId = requireNonNull(propCacheId, "a PropCacheId must be
supplied");
+ this.parent = requireNonNull(parent, "An AccumuloConfiguration parent must
be supplied");
+
+ this.propStore =
+ requireNonNull(context.getPropStore(), "The PropStore must be supplied
and exist");
+
+ propStore.registerAsListener(propCacheId, this);
+
+ snapshotRef.set(updateSnapshot());
+
+ }
+
+ public long getDataVersion() {
+ var snapshot = snapshotRef.get();
+ if (snapshot == null) {
+ return updateSnapshot().getDataVersion();
+ }
+ return snapshot.getDataVersion();
+ }
+
+ /**
+ * The update count is the sum of the change count of this configuration and
the change counts of
+ * the parents. The count is used to detect if any changes occurred in the
configuration hierarchy
+ * and if the configuration needs to be recalculated to maintain consistency
with values in the
+ * backend store.
+ * <p>
+ * The count is required to be an increasing value.
+ */
+ @Override
+ public long getUpdateCount() {
+ long count = 0;
+ long dataVersion = 0;
+ for (AccumuloConfiguration p = this; p != null; p = p.getParent()) {
+ if (p instanceof ZooBasedConfiguration) {
+ dataVersion = ((ZooBasedConfiguration) p).getDataVersion();
+ } else {
+ dataVersion = p.getUpdateCount();
+ }
+ count += dataVersion;
+ }
+
+ log.trace("update count result for: {} - data version: {} update: {}",
propCacheId, dataVersion,
+ count);
+ return count;
+ }
+
+ @Override
+ public AccumuloConfiguration getParent() {
+ return parent;
+ }
+
+ public PropCacheId getCacheId() {
+ return propCacheId;
+ }
+
+ @Override
+ public @Nullable String get(final Property property) {
+ Map<String,String> props = getSnapshot();
+ String value = props.get(property.getKey());
+ if (value != null) {
+ return value;
+ }
+ AccumuloConfiguration parent = getParent();
+ if (parent != null) {
+ return parent.get(property);
+ }
+ return null;
+ }
+
+ @Override
+ public void getProperties(final Map<String,String> props, final
Predicate<String> filter) {
+
+ parent.getProperties(props, filter);
+
+ Map<String,String> theseProps = getSnapshot();
+
+ log.trace("getProperties() for: {} filter: {}, have: {}, passed: {}",
getCacheId(), filter,
+ theseProps, props);
+
+ for (Map.Entry<String,String> p : theseProps.entrySet()) {
+ if (filter.test(p.getKey()) && p.getValue() != null) {
+ log.trace("passed filter - add to map: {} = {}", p.getKey(),
p.getValue());
+ props.put(p.getKey(), p.getValue());
+ }
+ }
+ }
+
+ @Override
+ public boolean isPropertySet(final Property property) {
+
+ Map<String,String> theseProps = getSnapshot();
+
+ if (theseProps.get(property.getKey()) != null) {
+ return true;
+ }
+
+ return getParent().isPropertySet(property);
+
+ }
+
+ public Map<String,String> getSnapshot() {
+ if (snapshotRef.get() == null) {
+ return updateSnapshot().getProps();
+ }
+ return snapshotRef.get().getProps();
+ }
+
+ @Override
+ public void invalidateCache() {
+ snapshotRef.set(null);
+ }
+
+ private final Lock updateLock = new ReentrantLock();
+
+ private @NonNull PropSnapshot updateSnapshot() throws PropStoreException {
+
+ PropSnapshot localSnapshot = snapshotRef.get();
Review comment:
This class has a reference to `ZooStore`, which uses `Caffeine` to cache
the entries from ZooKeeper. Then in addition to that, this class keeps another
cached copy of the properties? Is that right?
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java
##########
@@ -0,0 +1,584 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.conf.store.impl;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.BiFunction;
+
+import org.apache.accumulo.core.data.InstanceId;
+import org.apache.accumulo.core.metrics.MetricsUtil;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.conf.codec.VersionedPropCodec;
+import org.apache.accumulo.server.conf.codec.VersionedPropGzipCodec;
+import org.apache.accumulo.server.conf.codec.VersionedProperties;
+import org.apache.accumulo.server.conf.store.PropCache;
+import org.apache.accumulo.server.conf.store.PropCacheId;
+import org.apache.accumulo.server.conf.store.PropChangeListener;
+import org.apache.accumulo.server.conf.store.PropStore;
+import org.apache.accumulo.server.conf.store.PropStoreException;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.github.benmanes.caffeine.cache.Ticker;
+import com.google.common.annotations.VisibleForTesting;
+
+public class ZooPropStore implements PropStore, PropChangeListener {
+
+ private final static Logger log =
LoggerFactory.getLogger(ZooPropStore.class);
+ private final static VersionedPropCodec codec =
VersionedPropGzipCodec.codec(true);
+
+ private final ZooReaderWriter zrw;
+ private final PropStoreWatcher propStoreWatcher;
+ private final PropCache cache;
+ private final PropStoreMetrics cacheMetrics = new PropStoreMetrics();
+ private final ReadyMonitor zkReadyMon;
+
+ /**
+ * Create instance using ZooPropStore.Builder
+ *
+ * @param instanceId
+ * the instance id
+ * @param zrw
+ * a wrapper set of utilities for accessing ZooKeeper.
+ * @param readyMonitor
+ * coordination utility for ZooKeeper connection status.
+ * @param propStoreWatcher
+ * an extended ZooKeeper watcher
+ * @param ticker
+ * a synthetic clock used for testing.
+ */
+ private ZooPropStore(final InstanceId instanceId, final ZooReaderWriter zrw,
+ final ReadyMonitor readyMonitor, final PropStoreWatcher propStoreWatcher,
+ final Ticker ticker) {
+
+ this.zrw = zrw;
+ this.zkReadyMon = readyMonitor;
+ this.propStoreWatcher = propStoreWatcher;
+
+ MetricsUtil.initializeProducers(cacheMetrics);
+
+ ZooPropLoader propLoader = new ZooPropLoader(zrw, codec, propStoreWatcher,
cacheMetrics);
+
+ if (ticker == null) {
+ cache = new CaffeineCache.Builder(propLoader, cacheMetrics).build();
+ } else {
+ cache = new CaffeineCache.Builder(propLoader,
cacheMetrics).withTicker(ticker).build();
+ }
+
+ try {
+ var path = ZooUtil.getRoot(instanceId);
+ if (zrw.exists(path, propStoreWatcher)) {
+ log.debug("Have a ZooKeeper connection and found instance node: {}",
instanceId);
+ zkReadyMon.setReady();
+ } else {
+ throw new IllegalStateException("Instance may not have been
initialized, root node: " + path
+ + " does not exist in ZooKeeper");
+ }
+ } catch (InterruptedException | KeeperException ex) {
+ throw new IllegalStateException("Failed to read root node " + instanceId
+ " from ZooKeeper",
+ ex);
+ }
+ }
+
+ public static VersionedPropCodec getCodec() {
+ return codec;
+ }
+
+ public static PropStore initialize(final InstanceId instanceId, final
ZooReaderWriter zrw) {
+ return new ZooPropStore.Builder(instanceId, zrw,
zrw.getSessionTimeout()).build();
Review comment:
Ok, I understand that, but we are not doing anything to protect
ourselves here. There is nothing to prevent multiple instances of these objects
from being created in the future except a code review. Could we at least make
the Builder methods `private`, remove the `public` modifier from this method,
and move this class to the same package from where it's called?
--
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]
