Allow modifying only content with FileSync

6 files changed, 44 insertions, 56 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java
index cba3912ce49..16eb1a9b509 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java
@@ -1,5 +1,4 @@
 // 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.node.admin.task.util.file;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
@@ -77,21 +76,21 @@ public class AttributeSync {
                 context,
                 "owner",
                 owner,
-                () -> currentAttributes.get().owner(),
+                () -> currentAttributes.getOrThrow().owner(),
                 path::setOwner);
 
         systemModified |= updateAttribute(
                 context,
                 "group",
                 group,
-                () -> currentAttributes.get().group(),
+                () -> currentAttributes.getOrThrow().group(),
                 path::setGroup);
 
         systemModified |= updateAttribute(
                 context,
                 "permissions",
                 permissions,
-                () -> currentAttributes.get().permissions(),
+                () -> currentAttributes.getOrThrow().permissions(),
                 path::setPermissions);
 
         return systemModified;
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCache.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCache.java
index 12a9609f89c..a35bc844b8f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCache.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCache.java
@@ -1,5 +1,4 @@
 // 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.node.admin.task.util.file;
 
 import java.util.Optional;
@@ -11,34 +10,23 @@ public class FileAttributesCache {
     private Optional<FileAttributes> attributes = Optional.empty();
 
     public FileAttributesCache(UnixPath path) {
-        this.path =  path;
+        this.path = path;
     }
 
-    public FileAttributes get() {
-        if (!attributes.isPresent()) {
-            attributes = Optional.of(path.getAttributes());
+    public Optional<FileAttributes> get() {
+        if (attributes.isEmpty()) {
+            attributes = path.getAttributesIfExists();
         }
 
-        return attributes.get();
+        return attributes;
     }
 
-    public FileAttributes forceGet() {
-        attributes = Optional.empty();
-        return get();
+    public FileAttributes getOrThrow() {
+        return get().orElseThrow();
     }
 
-    public boolean exists() {
-        if (attributes.isPresent()) {
-            return true;
-        }
-
-        Optional<FileAttributes> attributes = path.getAttributesIfExists();
-        if (attributes.isPresent()) {
-            // Might as well update this.attributes
-            this.attributes = attributes;
-            return true;
-        } else {
-            return false;
-        }
+    public Optional<FileAttributes> forceGet() {
+        attributes = Optional.empty();
+        return get();
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileContentCache.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileContentCache.java
index 43c9f7729ef..974ab68dc1c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileContentCache.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileContentCache.java
@@ -1,5 +1,4 @@
 // 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.node.admin.task.util.file;
 
 import java.time.Instant;
@@ -21,7 +20,7 @@ class FileContentCache {
     }
 
     byte[] get(Instant lastModifiedTime) {
-        if (!value.isPresent() || lastModifiedTime.compareTo(modifiedTime.get()) > 0) {
+        if (modifiedTime.isEmpty() || lastModifiedTime.isAfter(modifiedTime.get())) {
             value = Optional.of(path.readBytes());
             modifiedTime = Optional.of(lastModifiedTime);
         }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java
index dc13ea1c9ab..7a11ecdf6ab 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java
@@ -1,18 +1,14 @@
 // 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.node.admin.task.util.file;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 
-import java.nio.file.FileSystems;
-import java.nio.file.Files;
 import java.nio.file.Path;
+import java.time.Instant;
 import java.util.Arrays;
 import java.util.Optional;
 import java.util.logging.Logger;
 
-import static com.yahoo.yolean.Exceptions.uncheck;
-
 /**
  * Class to minimize resource usage with repetitive and mostly identical, idempotent, and
  * mutating file operations, e.g. setting file content, setting owner, etc.
@@ -27,10 +23,12 @@ public class FileSync {
 
     private final UnixPath path;
     private final FileContentCache contentCache;
+    private final FileAttributesCache attributesCache;
 
     public FileSync(Path path) {
         this.path = new UnixPath(path);
         this.contentCache = new FileContentCache(this.path);
+        this.attributesCache = new FileAttributesCache(this.path);
     }
 
     public boolean convergeTo(TaskContext taskContext, PartialFileData partialFileData) {
@@ -46,38 +44,41 @@ public class FileSync {
      *         system is only modified if necessary (different).
      */
     public boolean convergeTo(TaskContext taskContext, PartialFileData partialFileData, boolean atomicWrite) {
-        FileAttributesCache currentAttributes = new FileAttributesCache(path);
+        boolean modifiedSystem = false;
 
-        boolean modifiedSystem = maybeUpdateContent(taskContext, partialFileData.getContent(), currentAttributes, atomicWrite);
+        if (partialFileData.getContent().isPresent())
+            modifiedSystem |= convergeTo(taskContext, partialFileData.getContent().get(), atomicWrite);
 
         AttributeSync attributeSync = new AttributeSync(path.toPath()).with(partialFileData);
-        modifiedSystem |= attributeSync.converge(taskContext, currentAttributes);
+        modifiedSystem |= attributeSync.converge(taskContext, this.attributesCache);
 
         return modifiedSystem;
     }
 
-    private boolean maybeUpdateContent(TaskContext taskContext,
-                                       Optional<byte[]> content,
-                                       FileAttributesCache currentAttributes,
-                                       boolean atomicWrite) {
-        if (!content.isPresent()) {
-            return false;
-        }
+    /**
+     * CPU, I/O, and memory usage is optimized for repeated calls with the same argument.
+     *
+     * @param atomicWrite Whether to write updates to a temporary file in the same directory, and atomically move it
+     *                    to path. Ensures the file cannot be read while in the middle of writing it.
+     * @return true if the content was written. Only modified if necessary (different).
+     */
+    public boolean convergeTo(TaskContext taskContext, byte[] content, boolean atomicWrite) {
+        Optional<Instant> lastModifiedTime = attributesCache.forceGet().map(FileAttributes::lastModifiedTime);
 
-        if (!currentAttributes.exists()) {
+        if (lastModifiedTime.isEmpty()) {
             taskContext.recordSystemModification(logger, "Creating file " + path);
             path.createParents();
-            writeBytes(content.get(), atomicWrite);
-            contentCache.updateWith(content.get(), currentAttributes.forceGet().lastModifiedTime());
+            writeBytes(content, atomicWrite);
+            contentCache.updateWith(content, attributesCache.forceGet().orElseThrow().lastModifiedTime());
             return true;
         }
 
-        if (Arrays.equals(content.get(), contentCache.get(currentAttributes.get().lastModifiedTime()))) {
+        if (Arrays.equals(content, contentCache.get(attributesCache.getOrThrow().lastModifiedTime()))) {
             return false;
         } else {
             taskContext.recordSystemModification(logger, "Patching file " + path);
-            writeBytes(content.get(), atomicWrite);
-            contentCache.updateWith(content.get(), currentAttributes.forceGet().lastModifiedTime());
+            writeBytes(content, atomicWrite);
+            contentCache.updateWith(content, attributesCache.forceGet().orElseThrow().lastModifiedTime());
             return true;
         }
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java
index 5f72ed7e9b8..e2366470f61 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java
@@ -19,12 +19,14 @@ public class MakeDirectory {
 
     private final UnixPath path;
     private final AttributeSync attributeSync;
+    private final FileAttributesCache attributesCache;
 
     private boolean createParents = false;
 
     public MakeDirectory(Path path) {
         this.path = new UnixPath(path);
         this.attributeSync = new AttributeSync(path);
+        this.attributesCache = new FileAttributesCache(this.path);
     }
 
     /**
@@ -42,8 +44,8 @@ public class MakeDirectory {
     public boolean converge(TaskContext context) {
         boolean systemModified = false;
 
-        FileAttributesCache attributes = new FileAttributesCache(path);
-        if (attributes.exists()) {
+        Optional<FileAttributes> attributes = attributesCache.forceGet();
+        if (attributes.isPresent()) {
             if (!attributes.get().isDirectory()) {
                 throw new UncheckedIOException(new NotDirectoryException(path.toString()));
             }
@@ -65,7 +67,7 @@ public class MakeDirectory {
             }
         }
 
-        systemModified |= attributeSync.converge(context, attributes);
+        systemModified |= attributeSync.converge(context, attributesCache);
 
         return systemModified;
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCacheTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCacheTest.java
index 06192c9f308..88b33a7f9a8 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCacheTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileAttributesCacheTest.java
@@ -1,5 +1,4 @@
 // 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.node.admin.task.util.file;
 
 import org.junit.Test;
@@ -22,17 +21,17 @@ public class FileAttributesCacheTest {
         FileAttributesCache cache = new FileAttributesCache(unixPath);
 
         when(unixPath.getAttributesIfExists()).thenReturn(Optional.empty());
-        assertFalse(cache.exists());
+        assertFalse(cache.get().isPresent());
         verify(unixPath, times(1)).getAttributesIfExists();
         verifyNoMoreInteractions(unixPath);
 
         FileAttributes attributes = mock(FileAttributes.class);
         when(unixPath.getAttributesIfExists()).thenReturn(Optional.of(attributes));
-        assertTrue(cache.exists());
+        assertTrue(cache.get().isPresent());
         verify(unixPath, times(1 + 1)).getAttributesIfExists();
         verifyNoMoreInteractions(unixPath);
 
-        assertEquals(attributes, cache.get());
+        assertEquals(attributes, cache.getOrThrow());
         verifyNoMoreInteractions(unixPath);
     }
 }
\ No newline at end of file