diff options
15 files changed, 59 insertions, 63 deletions
diff --git a/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def b/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def index 45815284dce..b054f434322 100644 --- a/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def +++ b/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def @@ -9,3 +9,6 @@ tenantContainerImage string default="" # Whether to cache data read from ZooKeeper in-memory. useCuratorClientCache bool default=false + +# The number of Node objects to cache in-memory. +nodeCacheSize long default=3000 diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index 17832ff9550..7f54fff5c70 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -84,7 +84,8 @@ public class NodeRepository extends AbstractComponent { flagSource, metricsDb, config.useCuratorClientCache(), - zone.environment().isProduction() && !zone.getCloud().dynamicProvisioning() && !zone.system().isCd() ? 1 : 0); + zone.environment().isProduction() && !zone.getCloud().dynamicProvisioning() && !zone.system().isCd() ? 1 : 0, + config.nodeCacheSize()); } /** @@ -102,13 +103,14 @@ public class NodeRepository extends AbstractComponent { FlagSource flagSource, MetricsDb metricsDb, boolean useCuratorClientCache, - int spareCount) { + int spareCount, + long nodeCacheSize) { if (provisionServiceProvider.getHostProvisioner().isPresent() != zone.getCloud().dynamicProvisioning()) throw new IllegalArgumentException(String.format( "dynamicProvisioning property must be 1-to-1 with availability of HostProvisioner, was: dynamicProvisioning=%s, hostProvisioner=%s", zone.getCloud().dynamicProvisioning(), provisionServiceProvider.getHostProvisioner().map(__ -> "present").orElse("empty"))); - this.db = new CuratorDatabaseClient(flavors, curator, clock, useCuratorClientCache); + this.db = new CuratorDatabaseClient(flavors, curator, clock, useCuratorClientCache, nodeCacheSize); this.zone = zone; this.clock = clock; this.nodes = new Nodes(db, zone, clock); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java index acf7aa5c28e..ca14a1be4c4 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java @@ -133,7 +133,7 @@ public class MetricsReporter extends NodeRepositoryMaintainer { } private void updateCacheMetrics() { - CacheStats nodeCacheStats = nodeRepository().database().objectCacheStats(); + CacheStats nodeCacheStats = nodeRepository().database().nodeSerializerCacheStats(); metric.set("cache.nodeObject.hitRate", nodeCacheStats.hitRate(), null); metric.set("cache.nodeObject.evictionCount", nodeCacheStats.evictionCount(), null); metric.set("cache.nodeObject.size", nodeCacheStats.size(), null); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java index 83cfe72d9e9..659636de581 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.provision.persistence; import com.google.common.cache.AbstractCache; +import com.google.common.collect.ImmutableList; import com.yahoo.config.provision.HostName; import com.yahoo.path.Path; import com.yahoo.transaction.NestedTransaction; @@ -9,14 +10,12 @@ import com.yahoo.vespa.curator.Curator; import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.curator.recipes.CuratorCounter; import com.yahoo.vespa.curator.transaction.CuratorTransaction; -import org.apache.zookeeper.data.Stat; import java.time.Duration; import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.OptionalInt; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; @@ -156,7 +155,7 @@ public class CuratorDatabase { @Override public List<String> getChildren(Path path) { - return get(children, path, () -> List.copyOf(curator.getChildren(path))); + return get(children, path, () -> ImmutableList.copyOf(curator.getChildren(path))); } @Override @@ -164,11 +163,6 @@ public class CuratorDatabase { return get(data, path, () -> curator.getData(path)).map(data -> Arrays.copyOf(data, data.length)); } - @Override - public OptionalInt getVersion(Path path) { - return curator.getStat(path).stream().mapToInt(Stat::getVersion).findFirst(); - } - private <T> T get(Map<Path, T> values, Path path, Supplier<T> loader) { return values.compute(path, (key, value) -> { if (value == null) { @@ -208,13 +202,10 @@ public class CuratorDatabase { List<String> getChildren(Path path); /** - * Returns a copy of the content of this child - which may be empty. + * Returns the a copy of the content of this child - which may be empty. */ Optional<byte[]> getData(Path path); - /** Returns the current version of data stored in given path, if path exists. This should never be cached */ - OptionalInt getVersion(Path path); - } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java index 37b2f9ba665..379bb2566df 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java @@ -1,8 +1,6 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.persistence; -import com.google.common.cache.AbstractCache; -import com.yahoo.collections.Pair; import com.yahoo.component.Version; import com.yahoo.concurrent.UncheckedTimeoutException; import com.yahoo.config.provision.ApplicationId; @@ -37,9 +35,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.OptionalInt; import java.util.TreeMap; -import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; import java.util.function.Predicate; import java.util.logging.Level; @@ -80,13 +76,8 @@ public class CuratorDatabaseClient { private final Clock clock; private final CuratorCounter provisionIndexCounter; - // Cache Node objects and their znode version. The cache is invalidated if node data is changed, i.e. the znode - // version is changed. This will grow to keep all Node objects in memory, which should be fine. - private final Map<Path, Pair<Integer, Node>> cachedNodes = new ConcurrentHashMap<>(); - private final AbstractCache.SimpleStatsCounter cacheStats = new AbstractCache.SimpleStatsCounter(); - - public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, boolean useCache) { - this.nodeSerializer = new NodeSerializer(flavors); + public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, boolean useCache, long nodeCacheSize) { + this.nodeSerializer = new NodeSerializer(flavors, nodeCacheSize); this.db = new CuratorDatabase(curator, root, useCache); this.clock = clock; this.provisionIndexCounter = new CuratorCounter(curator, root.append("provisionIndexCounter")); @@ -276,27 +267,9 @@ public class CuratorDatabaseClient { if (states.length == 0) states = Node.State.values(); for (Node.State state : states) { - Path path = toPath(state, hostname); - OptionalInt version = session.getVersion(path); - if (version.isEmpty()) { - continue; - } - Pair<Integer, Node> versionAndNode = cachedNodes.compute(path, (ignored, old) -> { - if (old != null && old.getFirst() == version.getAsInt()) { - cacheStats.recordHits(1); - return old; // Cached entry has correct version - } else { - cacheStats.recordMisses(1); - } - Optional<byte[]> data = session.getData(path); - if (data.isEmpty()) { - return null; // Node disappeared after we read the version - } - return new Pair<>(version.getAsInt(), nodeSerializer.fromJson(state, data.get())); - }); - if (versionAndNode != null) { - return Optional.of(versionAndNode.getSecond()); - } + Optional<byte[]> nodeData = session.getData(toPath(state, hostname)); + if (nodeData.isPresent()) + return nodeData.map((data) -> nodeSerializer.fromJson(state, data)); } return Optional.empty(); } @@ -552,9 +525,8 @@ public class CuratorDatabaseClient { return db.cacheStats(); } - public CacheStats objectCacheStats() { - var snapshot = this.cacheStats.snapshot(); - return new CacheStats(snapshot.hitRate(), snapshot.evictionCount(), cachedNodes.size()); + public CacheStats nodeSerializerCacheStats() { + return nodeSerializer.cacheStats(); } private <T> Optional<T> read(Path path, Function<byte[], T> mapper) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java index 2424fb92b57..cd1b786afd1 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java @@ -1,7 +1,11 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.persistence; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.Hashing; +import com.google.common.util.concurrent.UncheckedExecutionException; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; @@ -35,11 +39,13 @@ import com.yahoo.vespa.hosted.provision.node.Status; import com.yahoo.vespa.hosted.provision.node.TrustStoreItem; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.time.Instant; import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; /** @@ -122,10 +128,17 @@ public class NodeSerializer { private static final String fingerprintKey = "fingerprint"; private static final String expiresKey = "expires"; + // A cache of deserialized Node objects. The cache is keyed on the hash of serialized node data. + // + // Deserializing a Node from slime is expensive, and happens frequently. Node instances that have already been + // deserialized are returned from this cache instead of being deserialized again. + private final Cache<Long, Node> cache; + // ---------------- Serialization ---------------------------------------------------- - public NodeSerializer(NodeFlavors flavors) { + public NodeSerializer(NodeFlavors flavors, long cacheSize) { this.flavors = flavors; + this.cache = CacheBuilder.newBuilder().maximumSize(cacheSize).recordStats().build(); } public byte[] toJson(Node node) { @@ -139,6 +152,12 @@ public class NodeSerializer { } } + /** Returns cache statistics for this serializer */ + public CacheStats cacheStats() { + var stats = cache.stats(); + return new CacheStats(stats.hitRate(), stats.evictionCount(), cache.size()); + } + private void toSlime(Node node, Cursor object) { object.setString(hostnameKey, node.hostname()); toSlime(node.ipConfig().primary(), object.setArray(ipAddressesKey)); @@ -235,7 +254,15 @@ public class NodeSerializer { // ---------------- Deserialization -------------------------------------------------- public Node fromJson(Node.State state, byte[] data) { - return nodeFromSlime(state, SlimeUtils.jsonToSlime(data).get()); + long key = Hashing.sipHash24().newHasher() + .putString(state.name(), StandardCharsets.UTF_8) + .putBytes(data).hash() + .asLong(); + try { + return cache.get(key, () -> nodeFromSlime(state, SlimeUtils.jsonToSlime(data).get())); + } catch (ExecutionException e) { + throw new UncheckedExecutionException(e); + } } private Node nodeFromSlime(Node.State state, Inspector object) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java index c1e26099ee4..1a2d5294aa5 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java @@ -71,7 +71,7 @@ public class MockNodeRepository extends NodeRepository { new InMemoryFlagSource(), new MemoryMetricsDb(Clock.fixed(Instant.ofEpochMilli(123), ZoneId.of("Z"))), true, - 0); + 0, 1000); this.flavors = flavors; curator.setZooKeeperEnsembleConnectionSpec("cfg1:1234,cfg2:1234,cfg3:1234"); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java index 1287c073981..b391292884f 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java @@ -48,7 +48,7 @@ public class NodeRepositoryTester { new InMemoryFlagSource(), new MemoryMetricsDb(clock), true, - 0); + 0, 1000); } public NodeRepository nodeRepository() { return nodeRepository; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java index 0944138e06c..0e732984227 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java @@ -136,7 +136,7 @@ public class RealDataScenarioTest { } private static void initFromZk(NodeRepository nodeRepository, Path pathToZkSnapshot) { - NodeSerializer nodeSerializer = new NodeSerializer(nodeRepository.flavors()); + NodeSerializer nodeSerializer = new NodeSerializer(nodeRepository.flavors(), 1000); AtomicReference<Node.State> state = new AtomicReference<>(); Pattern zkNodePathPattern = Pattern.compile(".?/provision/v1/([a-z]+)/[a-z0-9.-]+\\.(com|cloud).?"); Consumer<String> consumer = input -> { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java index 1c9532e4665..e06bdba90fb 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java @@ -74,7 +74,7 @@ public class CapacityCheckerTester { new InMemoryFlagSource(), new MemoryMetricsDb(clock), true, - 0); + 0, 1000); } private void updateCapacityChecker() { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java index eda5d201949..8c649243d61 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java @@ -124,9 +124,9 @@ public class MetricsReporterTest { expectedMetrics.put("cache.nodeObject.size", 2L); nodeRepository.nodes().list(); - expectedMetrics.put("cache.curator.hitRate", 0.5D); + expectedMetrics.put("cache.curator.hitRate", 0.52D); expectedMetrics.put("cache.curator.evictionCount", 0L); - expectedMetrics.put("cache.curator.size", 11L); + expectedMetrics.put("cache.curator.size", 12L); tester.clock().setInstant(Instant.ofEpochSecond(124)); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java index ca117ab1395..373bfe20162 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java @@ -268,7 +268,7 @@ public class SpareCapacityMaintainerTest { new InMemoryFlagSource(), new MemoryMetricsDb(clock), true, - 1); + 1, 1000); deployer = new MockDeployer(nodeRepository); maintainer = new SpareCapacityMaintainer(deployer, nodeRepository, metric, Duration.ofDays(1), maxIterations); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java index 5a04986338d..f11cfa9a15d 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java @@ -24,7 +24,7 @@ public class CuratorDatabaseClientTest { private final Curator curator = new MockCurator(); private final CuratorDatabaseClient zkClient = new CuratorDatabaseClient( - FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), true); + FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), true, 1000); @Test public void can_read_stored_host_information() throws Exception { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java index 31bf8ee43e2..3de3f4139d1 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java @@ -56,7 +56,7 @@ import static org.junit.Assert.assertTrue; public class NodeSerializerTest { private final NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default", "large", "ugccloud-container"); - private final NodeSerializer nodeSerializer = new NodeSerializer(nodeFlavors); + private final NodeSerializer nodeSerializer = new NodeSerializer(nodeFlavors, 1000); private final ManualClock clock = new ManualClock(); @Test diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java index 9a709748c1f..c478840780f 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java @@ -115,7 +115,8 @@ public class ProvisioningTester { flagSource, new MemoryMetricsDb(clock), true, - spareCount); + spareCount, + 1000); this.orchestrator = orchestrator; this.provisioner = new NodeRepositoryProvisioner(nodeRepository, zone, |