Merge pull request #591 from pdowler/main

fix skipped update of data node timstamp update
opencadc · Sep 24, 2024 · 1e70e55 · 1e70e55
2 parents ef18195 + 32be4e4
commit 1e70e55
Show file tree

Hide file tree

Showing 9 changed files with 104 additions and 56 deletions.
diff --git a/cadc-inventory-db/build.gradle b/cadc-inventory-db/build.gradle
@@ -17,7 +17,7 @@ sourceCompatibility = 11
 
 group = 'org.opencadc'
 
-version = '1.0.3'
+version = '1.0.4'
 
 description = 'OpenCADC Storage Inventory database library'
 def git_url = 'https://github.com/opencadc/storage-inventory'

diff --git a/cadc-inventory-db/src/intTest/java/org/opencadc/vospace/db/DataNodeSizeWorkerTest.java b/cadc-inventory-db/src/intTest/java/org/opencadc/vospace/db/DataNodeSizeWorkerTest.java
@@ -107,7 +107,7 @@ public class DataNodeSizeWorkerTest {
 
     static {
         Log4jInit.setLevel("org.opencadc.inventory", Level.INFO);
-        Log4jInit.setLevel("org.opencadc.inventory.db", Level.DEBUG);
+        Log4jInit.setLevel("org.opencadc.inventory.db", Level.INFO);
         Log4jInit.setLevel("ca.nrc.cadc.db", Level.INFO);
         Log4jInit.setLevel("org.opencadc.vospace", Level.INFO);
         Log4jInit.setLevel("org.opencadc.vospace.db", Level.INFO);
@@ -221,11 +221,7 @@ private void testSyncArtifact(boolean isStorageSite) throws Exception {
             expected.storageLocation.storageBucket = "X";
         }
         log.info("expected: " + expected);
-
         artifactDAO.put(expected);
-        Artifact actualArtifact = artifactDAO.get(expected.getID());
-        Assert.assertNotNull(actualArtifact);
-        Assert.assertEquals(expected.getContentLength(), actualArtifact.getContentLength());
 
         String hsName = "ArtifactSize";
         URI resourceID = URI.create("ivo://myorg.org/vospace");
@@ -237,6 +233,8 @@ private void testSyncArtifact(boolean isStorageSite) throws Exception {
         log.info("*** DataNodeSizeWorker START");
         asWorker.run();
         log.info("*** DataNodeSizeWorker DONE");
+        hs = harvestStateDAO.get(hsName, resourceID);
+        log.info("HarvestState: " + hs.curLastModified.getTime());
 
         actual = (DataNode)nodeDAO.get(orig.getID());
         Assert.assertNotNull(actual);
@@ -246,24 +244,51 @@ private void testSyncArtifact(boolean isStorageSite) throws Exception {
         Thread.sleep(100L);
 
         // update the artifact only
-        artifactDAO.delete(actualArtifact.getID());
-        Artifact modified = new Artifact(expected.getURI(), expected.getMetaChecksum(), new Date(), 333L);
+        artifactDAO.delete(expected.getID());
+        Artifact m1 = new Artifact(expected.getURI(), expected.getContentChecksum(), new Date(), 333L);
         if (isStorageSite) {
-            modified.storageLocation = new StorageLocation(URI.create("id:" + UUID.randomUUID().toString()));
-            modified.storageLocation.storageBucket = "X";
+            m1.storageLocation = new StorageLocation(URI.create("id:" + UUID.randomUUID().toString()));
+            m1.storageLocation.storageBucket = "X";
         }
-        artifactDAO.put(modified);
+        artifactDAO.put(m1);
         actual = (DataNode)nodeDAO.get(orig.getID());
-        Assert.assertNotEquals(modified.getContentLength(), actual.bytesUsed);
+        Assert.assertNotEquals(m1.getContentLength(), actual.bytesUsed);
 
         // run the update
         log.info("*** DataNodeSizeWorker START");
         asWorker.run();
         log.info("*** DataNodeSizeWorker DONE");
+        hs = harvestStateDAO.get(hsName, resourceID);
+        log.info("HarvestState: " + hs.curLastModified.getTime());
+
+        actual = (DataNode)nodeDAO.get(orig.getID());
+        Assert.assertEquals(m1.getContentLength(), actual.bytesUsed);
+        final Date nodeLastMod = actual.getLastModified();
 
+        Thread.sleep(100L);
+
+        // replace the artifact but with the same contentLength to ensure timestamp change
+        artifactDAO.delete(m1.getID());
+        Artifact m2 = new Artifact(expected.getURI(), expected.getContentChecksum(), new Date(), 333L); // same size
+        if (isStorageSite) {
+            m2.storageLocation = new StorageLocation(URI.create("id:" + UUID.randomUUID().toString()));
+            m2.storageLocation.storageBucket = "X";
+        }
+        artifactDAO.put(m2);
+        log.info("replaced artifact: " + m2.getLastModified().getTime());
         actual = (DataNode)nodeDAO.get(orig.getID());
-        Assert.assertEquals(modified.getContentLength(), actual.bytesUsed);
+        log.info("             node: " + actual.getLastModified().getTime());
+        Assert.assertTrue("no timestamp change", actual.getLastModified().equals(nodeLastMod));
 
-    }
+        // run the update
+        log.info("*** DataNodeSizeWorker START");
+        asWorker.run();
+        log.info("*** DataNodeSizeWorker DONE");
+        hs = harvestStateDAO.get(hsName, resourceID);
+        log.info("HarvestState: " + hs.curLastModified.getTime());
 
+        actual = (DataNode)nodeDAO.get(orig.getID());
+        Assert.assertEquals(m2.getContentLength(), actual.bytesUsed);
+        Assert.assertTrue("timestamp change", actual.getLastModified().after(nodeLastMod));
+    }
 }
diff --git a/cadc-inventory-db/src/main/java/org/opencadc/vospace/db/DataNodeSizeWorker.java b/cadc-inventory-db/src/main/java/org/opencadc/vospace/db/DataNodeSizeWorker.java
@@ -154,7 +154,9 @@ public void run() {
             while (iter.hasNext()) {
                 Artifact artifact = iter.next();
                 DataNode node = nodeDAO.getDataNode(artifact.getURI());
-                if (node != null  && !artifact.getContentLength().equals(node.bytesUsed)) {
+                if (node != null) {
+                    boolean delta = !artifact.getContentLength().equals(node.bytesUsed); // size change
+                    delta = delta || artifact.getLastModified().after(node.getLastModified());
                     log.debug(artifact.getURI() + " len=" + artifact.getContentLength() + " -> " + node.getName());
                     tm.startTransaction();
                     try {
@@ -163,7 +165,7 @@ public void run() {
                             continue; // node gone - race condition
                         }
                         node.bytesUsed = artifact.getContentLength();
-                        nodeDAO.put(node);
+                        nodeDAO.put(node, delta); // delta forces lastModified update
                         tm.commitTransaction();
                         log.debug("ArtifactSyncWorker.updateDataNode id=" + node.getID() 
                                 + " bytesUsed=" + node.bytesUsed + " artifact.lastModified=" + df.format(artifact.getLastModified()));

diff --git a/cadc-inventory-db/src/main/java/org/opencadc/vospace/db/NodeDAO.java b/cadc-inventory-db/src/main/java/org/opencadc/vospace/db/NodeDAO.java
@@ -104,6 +104,11 @@ public void put(Node val) {
         super.put(val);
     }
 
+    // used by DataNodeWorker
+    public void put(Node val, boolean forceTimestampUdpate) {
+        super.put(val, false, forceTimestampUdpate);
+    }
+
     @Override
     public Node lock(Node n) {
         if (n == null) {

diff --git a/vault/VERSION b/vault/VERSION
@@ -4,6 +4,6 @@
 # tags with and without build number so operators use the versioned
 # tag but we always keep a timestamped tag in case a semantic tag gets
 # replaced accidentally
-VER=1.0.10
+VER=1.0.11
 TAGS="${VER} ${VER}-$(date -u +"%Y%m%dT%H%M%S")"
 unset VER
diff --git a/vault/build.gradle b/vault/build.gradle
@@ -35,7 +35,7 @@ dependencies {
     compile 'org.opencadc:cadc-log:[1.1.6,2.0)'
     compile 'org.opencadc:cadc-gms:[1.0.5,)'
     compile 'org.opencadc:cadc-rest:[1.3.16,)'
-    compile 'org.opencadc:cadc-vos:[2.0.6,)'
+    compile 'org.opencadc:cadc-vos:[2.0.7,)'
     compile 'org.opencadc:cadc-vos-server:[2.0.18,)'
     compile 'org.opencadc:cadc-vosi:[1.3.2,)'
     compile 'org.opencadc:cadc-uws:[1.0,)'
@@ -44,7 +44,7 @@ dependencies {
     compile 'org.opencadc:cadc-cdp:[1.2.3,)'
     compile 'org.opencadc:cadc-registry:[1.7.6,)'
     compile 'org.opencadc:cadc-inventory:[1.0.1,2.0)'
-    compile 'org.opencadc:cadc-inventory-db:[1.0.1,2.0)'
+    compile 'org.opencadc:cadc-inventory-db:[1.0.4,2.0)'
     compile 'org.opencadc:cadc-inventory-server:[0.3.0,1.0)'
     compile 'org.opencadc:cadc-permissions:[0.3.5,1.0)'
 

diff --git a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java
@@ -109,13 +109,10 @@
 import org.opencadc.vospace.NodeNotSupportedException;
 import org.opencadc.vospace.NodeProperty;
 import org.opencadc.vospace.VOS;
-import org.opencadc.vospace.VOSURI;
 import org.opencadc.vospace.db.NodeDAO;
 import org.opencadc.vospace.io.NodeWriter;
-import org.opencadc.vospace.server.LocalServiceURI;
 import org.opencadc.vospace.server.NodePersistence;
 import org.opencadc.vospace.server.Views;
-import org.opencadc.vospace.server.transfers.TransferGenerator;
 
 /**
  *
@@ -401,7 +398,11 @@ public Node get(ContainerNode parent, String name) throws TransientException {
                 // if DataNode.bytesUsed != Artifact.contentLength we update the cache
                 // this retains put+get consistency in a single-site deployed (with minoc)
                 // and may help hide some inconsistencies in child listing sizes
-                if (!a.getContentLength().equals(dn.bytesUsed)) {
+
+                // be consistent with DataNodeSizeWorker
+                boolean delta = !a.getContentLength().equals(dn.bytesUsed); // size change
+                delta = delta || a.getLastModified().after(dn.getLastModified());
+                if (delta) {
                     TransactionManager txn = dao.getTransactionManager();
                     try {
                         log.debug("starting node transaction");
@@ -412,7 +413,7 @@ public Node get(ContainerNode parent, String name) throws TransientException {
                         if (locked != null) {
                             dn = locked; // safer than accidentally using the wrong variable
                             dn.bytesUsed = a.getContentLength();
-                            dao.put(dn);
+                            dao.put(dn, delta);
                             ret = dn;
                         }
 
@@ -437,25 +438,21 @@ public Node get(ContainerNode parent, String name) throws TransientException {
                     }
                 }
 
-                Date d = ret.getLastModified();
-                Date cd = null;
-                if (ret.getLastModified().before(a.getLastModified())) {
-                    d = a.getLastModified();
-                }
-                if (d.before(a.getContentLastModified())) {
-                    // probably not possible
-                    d = a.getContentLastModified();
-                } else {
-                    cd = a.getContentLastModified();
+                // #date is always Node.lastModified for consistency with container listing
+                // TODO: some props like contentType are set on the artifact so those changes do not
+                //       cause a visible #date change; probably OK
+                ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_DATE, df.format(ret.getLastModified())));
+
+                // #content-date is only output if different from #date
+                if (!ret.getLastModified().equals(a.getContentLastModified())) {
+                    ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_CONTENTDATE, df.format(a.getContentLastModified())));
                 }
-                ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_DATE, df.format(d)));
-                if (cd != null) {
-                    ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_CONTENTDATE, df.format(cd)));
+
+                // only #md5 in VOSpace spec
+                if (a.getContentChecksum().getScheme().equalsIgnoreCase("md5")) {
+                    ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_CONTENTMD5, a.getContentChecksum().getSchemeSpecificPart()));
                 }
 
-                // assume MD5
-                ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_CONTENTMD5, a.getContentChecksum().getSchemeSpecificPart()));
-
                 if (a.contentEncoding != null) {
                     ret.getProperties().add(new NodeProperty(VOS.PROPERTY_URI_CONTENTENCODING, a.contentEncoding));
                 }
@@ -502,6 +499,7 @@ private class ChildNodeWrapper implements ResourceIterator<Node> {
 
         private final ContainerNode parent;
         private ResourceIterator<Node> childIter;
+        private boolean closedForException = false;
 
         private final IdentityManager identityManager = AuthenticationUtil.getIdentityManager();
         private final Map<Object, Subject> identCache = new TreeMap<>();
@@ -523,10 +521,7 @@ private class ChildNodeWrapper implements ResourceIterator<Node> {
 
         @Override
         public boolean hasNext() {
-            if (childIter != null) {
-                return childIter.hasNext();
-            }
-            return false;
+            return !closedForException && childIter != null && childIter.hasNext();
         }
 
         @Override
@@ -537,14 +532,25 @@ public Node next() {
             Node ret = childIter.next();
             ret.parent = parent;
 
-            // owner
-            Subject s = identCache.get(ret.ownerID);
-            if (s == null) {
-                s = identityManager.toSubject(ret.ownerID);
-                identCache.put(ret.ownerID, s);
+            try {
+                // owner
+                Subject s = identCache.get(ret.ownerID);
+                if (s == null) {
+                    s = identityManager.toSubject(ret.ownerID);
+                    identCache.put(ret.ownerID, s);
+                }
+                ret.owner = s;
+                ret.ownerDisplay = identityManager.toDisplayString(ret.owner);
+            } catch (RuntimeException ex) {
+                try {
+                    // abort
+                    close();
+                } catch (Exception cex) {
+                    log.error("failed to close iterator", ex);
+                }
+                closedForException = true;
+                throw ex;
             }
-            ret.owner = s;
-            ret.ownerDisplay = identityManager.toDisplayString(ret.owner);
 
             if (ret instanceof DataNode) {
                 DataNode dn = (DataNode) ret;

diff --git a/vault/src/main/java/org/opencadc/vault/files/GetAction.java b/vault/src/main/java/org/opencadc/vault/files/GetAction.java
@@ -111,7 +111,7 @@ public void doAction() throws Exception {
         boolean noArtifact = node.bytesUsed == null || node.bytesUsed == 0L;
         noArtifact = noArtifact && nodePersistence.preventNotFound && rn.artifact == null;
         if (noArtifact) {
-            // no file
+            // no artifact found by preventNotFound
             syncOutput.setCode(HttpURLConnection.HTTP_NO_CONTENT);
             return;
         }
@@ -126,6 +126,14 @@ public void doAction() throws Exception {
             rn.protos = tg.getEndpoints(targetURI, pullTransfer, null);
             rn.artifact = tg.resolvedArtifact; // currently unused at this point
         }
+        // check artifact again (!preventNotFound)
+        noArtifact = node.bytesUsed == null || node.bytesUsed == 0L;
+        noArtifact = noArtifact && rn.artifact == null;
+        if (noArtifact) {
+            // no file
+            syncOutput.setCode(HttpURLConnection.HTTP_NO_CONTENT);
+            return;
+        }
 
         if (rn.protos.isEmpty()) {
             throw new TransientException("No location found for file " + Utils.getPath(node));

diff --git a/vault/src/main/java/org/opencadc/vault/metadata/BackgroundLogInfo.java b/vault/src/main/java/org/opencadc/vault/metadata/BackgroundLogInfo.java
@@ -98,9 +98,11 @@ public void setOperation(String op) {
     }
 
     public void setLastModified(Date ts) {
-        DateFormat df = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC);
-        this.lastmodified = df.format(ts);
+        if (ts != null) {
+            DateFormat df = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC);
+            this.lastmodified = df.format(ts);
+        } else {
+            this.lastmodified = null;
+        }
     }
-
-
 }