Merge pull request #17474 from vespa-engine/revert-17469-mpolden/improve-cache

9 files changed, 58 insertions, 80 deletions

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 0e6146eae03..4cf531b55de 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
@@ -109,7 +109,7 @@ public class NodeRepository extends AbstractComponent {
                     "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, nodeCacheSize);
+        this.db = new CuratorDatabaseClient(flavors, curator, clock, zone, 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 6451a8623d4..8d8c7ef42f2 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
@@ -130,7 +130,7 @@ public class MetricsReporter extends NodeRepositoryMaintainer {
     }
 
     private void updateCacheMetrics() {
-        CacheStats nodeCacheStats = nodeRepository().database().nodeCacheStats();
+        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 7b73644be6f..fa5a72eea52 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
@@ -10,7 +10,6 @@ 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;
@@ -164,11 +163,6 @@ public class CuratorDatabase {
             return get(data, path, () -> curator.getData(path)).map(data -> Arrays.copyOf(data, data.length));
         }
 
-        @Override
-        public Optional<Stat> getStat(Path path) {
-            return curator.getStat(path);
-        }
-
         private <T> T get(Map<Path, T> values, Path path, Supplier<T> loader) {
             return values.compute(path, (key, value) -> {
                 if (value == null) {
@@ -202,15 +196,16 @@ public class CuratorDatabase {
 
     interface Session {
 
-        /** Returns the children of this path, which may be empty */
+        /**
+         * Returns the children of this path, which may be empty.
+         */
         List<String> getChildren(Path path);
 
-        /** Returns the 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 stat data of given path */
-        Optional<Stat> getStat(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 d65004911af..2d8ab74ce71 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,9 +1,6 @@
 // Copyright Verizon Media. 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.util.concurrent.UncheckedExecutionException;
 import com.google.common.util.concurrent.UncheckedTimeoutException;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
@@ -14,6 +11,7 @@ import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.TenantName;
+import com.yahoo.config.provision.Zone;
 import com.yahoo.path.Path;
 import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.transaction.Transaction;
@@ -29,7 +27,6 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.Status;
 import com.yahoo.vespa.hosted.provision.os.OsVersionChange;
-import org.apache.zookeeper.data.Stat;
 
 import java.time.Clock;
 import java.time.Duration;
@@ -39,11 +36,8 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.TreeMap;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.logging.Level;
@@ -83,19 +77,16 @@ public class CuratorDatabaseClient {
     private final NodeSerializer nodeSerializer;
     private final CuratorDatabase db;
     private final Clock clock;
+    private final Zone zone;
     private final CuratorCounter provisionIndexCounter;
 
-    // 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<Path, CachedNode> nodeCache;
-
-    public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, boolean useCache,
+    public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, boolean useCache,
                                  long nodeCacheSize) {
-        this.nodeSerializer = new NodeSerializer(flavors);
+        this.nodeSerializer = new NodeSerializer(flavors, nodeCacheSize);
+        this.zone = zone;
         this.db = new CuratorDatabase(curator, root, useCache);
         this.clock = clock;
         this.provisionIndexCounter = new CuratorCounter(curator, root.append("provisionIndexCounter").getAbsolute());
-        this.nodeCache = CacheBuilder.newBuilder().maximumSize(nodeCacheSize).recordStats().build();;
         initZK();
     }
 
@@ -163,7 +154,7 @@ public class CuratorDatabaseClient {
      * @return the nodes in their persisted state
      */
     public List<Node> writeTo(List<Node> nodes, Agent agent, Optional<String> reason) {
-        if (nodes.isEmpty()) return List.of();
+        if (nodes.isEmpty()) return Collections.emptyList();
 
         List<Node> writtenNodes = new ArrayList<>(nodes.size());
 
@@ -196,7 +187,7 @@ public class CuratorDatabaseClient {
         }
     }
     public Node writeTo(Node.State toState, Node node, Agent agent, Optional<String> reason) {
-        return writeTo(toState, List.of(node), agent, reason).get(0);
+        return writeTo(toState, Collections.singletonList(node), agent, reason).get(0);
     }
 
     /**
@@ -264,7 +255,7 @@ public class CuratorDatabaseClient {
      *
      * @return the nodes in a mutable list owned by the caller
      */
-    public List<Node> readNodes(Node.State... states) {
+    public List<Node> readNodes(Node.State ... states) {
         List<Node> nodes = new ArrayList<>();
         if (states.length == 0)
             states = Node.State.values();
@@ -282,29 +273,13 @@ public class CuratorDatabaseClient {
      * Returns a particular node, or empty if this node is not in any of the given states.
      * If no states are given this returns the node if it is present in any state.
      */
-    private Optional<Node> readNode(CuratorDatabase.Session session, String hostname, Node.State... states) {
+    public Optional<Node> readNode(CuratorDatabase.Session session, String hostname, Node.State ... states) {
         if (states.length == 0)
             states = Node.State.values();
         for (Node.State state : states) {
-            Path path = toPath(state, hostname);
-            Optional<Stat> stat = session.getStat(path);
-            if (stat.isPresent()) {
-                int currentVersion = stat.get().getVersion();
-                try {
-                    Callable<CachedNode> loader = () -> new CachedNode(session.getData(path)
-                                                                              .map(data -> nodeSerializer.fromJson(state, data))
-                                                                              .get(),
-                                                                       currentVersion);
-                    CachedNode cachedNode = nodeCache.get(path, loader);
-                    if (cachedNode.version != currentVersion) {
-                        nodeCache.invalidate(path);
-                        cachedNode = nodeCache.get(path, loader);
-                    }
-                    return Optional.of(cachedNode.node);
-                } catch (ExecutionException e) {
-                    throw new UncheckedExecutionException(e);
-                }
-            }
+            Optional<byte[]> nodeData = session.getData(toPath(state, hostname));
+            if (nodeData.isPresent())
+                return nodeData.map((data) -> nodeSerializer.fromJson(state, data));
         }
         return Optional.empty();
     }
@@ -313,7 +288,7 @@ public class CuratorDatabaseClient {
      * Returns a particular node, or empty if this noe is not in any of the given states.
      * If no states are given this returns the node if it is present in any state.
      */
-    public Optional<Node> readNode(String hostname, Node.State... states) {
+    public Optional<Node> readNode(String hostname, Node.State ... states) {
         return readNode(db.getSession(), hostname, states);
     }
 
@@ -557,15 +532,12 @@ public class CuratorDatabaseClient {
                         .collect(Collectors.toList());
     }
 
-    /** Returns cache statistics for curator cache */
     public CacheStats cacheStats() {
         return db.cacheStats();
     }
 
-    /** Returns cache statistics for the node cache */
-    public CacheStats nodeCacheStats() {
-        var stats = nodeCache.stats();
-        return new CacheStats(stats.hitRate(), stats.evictionCount(), nodeCache.size());
+    public CacheStats nodeSerializerCacheStats() {
+        return nodeSerializer.cacheStats();
     }
 
     private <T> Optional<T> read(Path path, Function<byte[], T> mapper) {
@@ -577,21 +549,4 @@ public class CuratorDatabaseClient {
                                             : CuratorOperations.create(path.getAbsolute(), data);
     }
 
-    private static class CachedNode {
-
-        private final Node node;
-        private final int version;
-
-        private CachedNode(Node node, int version) {
-            this.node = Objects.requireNonNull(node);
-            this.version = version;
-        }
-
-        @Override
-        public String toString() {
-            return node + " version " + version;
-        }
-
-    }
-
 }
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 1c6ead5de5c..97b9393bdd4 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 2018 Yahoo Holdings. 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;
@@ -33,11 +37,13 @@ import com.yahoo.vespa.hosted.provision.node.Reports;
 import com.yahoo.vespa.hosted.provision.node.Status;
 
 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;
 
 /**
@@ -114,10 +120,17 @@ public class NodeSerializer {
     // Network port fields
     private static final String networkPortsKey = "networkPorts";
 
+    // 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) {
@@ -131,6 +144,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));
@@ -217,7 +236,15 @@ public class NodeSerializer {
     // ---------------- Deserialization --------------------------------------------------
 
     public Node fromJson(Node.State state, byte[] data) {
-        return nodeFromSlime(state, SlimeUtils.jsonToSlime(data).get());
+        var 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/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java
index 1f5847814e1..b812a547ede 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
@@ -104,7 +104,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/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java
index 0c8b340ce7c..968427f0781 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
@@ -122,9 +122,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/persistence/CuratorDatabaseClientTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
index 4cadd6dac18..f47fb7f23be 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
@@ -6,6 +6,7 @@ import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.TenantName;
+import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.hosted.provision.Node;
@@ -24,7 +25,7 @@ public class CuratorDatabaseClientTest {
 
     private final Curator curator = new MockCurator();
     private final CuratorDatabaseClient zkClient = new CuratorDatabaseClient(
-            FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), true, 1000);
+            FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), Zone.defaultZone(), 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 eb5ccd89e89..523ceeb94ce 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
@@ -54,7 +54,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