Skip to content

Commit

Permalink
NXP-30271: keep tmp files during key/value store GC
Browse files Browse the repository at this point in the history
  • Loading branch information
@efge
efge committed Mar 24, 2021
1 parent 30e79c4 commit 8d4d5fb
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 18 deletions.
57 changes: 47 additions & 10 deletions modules/core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/blob/CachingBlobStore.java
View file
Expand Up @@ -69,7 +69,7 @@ public class CachingBlobStore extends AbstractBlobStore {
protected long clearOldBlobsLastTime;

// not a constant for tests
protected long clearOldBlobsInterval = Duration.ofSeconds(5).toMillis();
protected long clearOldBlobsInterval = Duration.ofMinutes(1).toMillis();

// not a constant for tests
protected Clock clock = Clock.systemUTC();
Expand Down Expand Up @@ -278,26 +278,41 @@ protected void clearOldBlobs() {
* @since 11.5
*/
protected void clearOldBlobsNow() {
long maxSize = cacheConfig.maxSize;
long maxCount = cacheConfig.maxCount;
long minAgeMillis = cacheConfig.minAge * 1000;
long threshold = clock.millis() - minAgeMillis;
log.debug("clearOldBlobs starting, dir={} maxSize={}, maxCount={}, minAge={}s, threshold={}", cacheConfig.dir,
maxSize, maxCount, cacheConfig.minAge, threshold);

List<PathInfo> files = new ArrayList<>();
try (DirectoryStream<Path> ds = Files.newDirectoryStream(cacheConfig.dir)) {
for (Path path : ds) {
PathInfo pi;
try {
files.add(new PathInfo(path));
pi = new PathInfo(path);
} catch (NoSuchFileException e) {
log.trace("clearOldBlobs ignoring missing file: {}", path);
continue;
} catch (IOException e) {
log.error(e, e);
log.warn(e.getMessage());
continue;
}
if (cacheStore.pathStrategy.isTempFile(path)) {
log.trace("clearOldBlobs ignoring temporary file: {} (timestamp {})", path, pi.time);
continue;
}
files.add(pi);
}
} catch (IOException e) {
log.error(e, e);
log.warn(e.getMessage());
}
Collections.sort(files); // sort by most recent first

long maxSize = cacheConfig.maxSize;
long maxCount = cacheConfig.maxCount;
long minAgeMillis = cacheConfig.minAge * 1000;
log.debug("clearOldBlobs {} files to check", files.size());
long size = 0;
long count = 0;
long threshold = clock.millis() - minAgeMillis;
long recentCount = 0;
for (PathInfo pi : files) {
size += pi.size;
count++;
Expand All @@ -311,18 +326,40 @@ protected void clearOldBlobsNow() {
long time = Files.getLastModifiedTime(pi.path).toMillis();
if (time < threshold) {
// delete the file
log.trace(
"clearOldBlobs DELETING file: {} (timestamp {}, cumulative count {}, cumulative size {})",
pi.path, time, count, size);
Files.delete(pi.path);
size -= pi.size;
count--;
} else {
recentCount++;
log.trace("clearOldBlobs keeping file: {} because it's recent (timestamp {})", pi.path,
time);
}
} catch (IOException e) {
log.error(e, e);
log.warn(e.getMessage());
} finally {
unlock(pi.path);
}
} // else if failed to lock ignore the file
} else {
log.trace("clearOldBlobs skipping file: {} because it's already locked", pi.path);
}
} else {
recentCount++;
log.trace("clearOldBlobs keeping file: {} because it's recent (timestamp {})", pi.path, pi.time);
}
} else {
log.trace("clearOldBlobs keeping file: {}", pi.path);
}
}
if (log.isDebugEnabled()) {
if (maxSize == 0) {
maxSize = 1; // shouldn't happen, but don't divide by zero
}
log.debug(String.format(
"clearOldBlobs done (keeping %d files out of %d (including %d recent), cache fill ratio now %.1f%%)",
count, files.size(), recentCount, 100d * size / maxSize));
}
}

Expand Down
32 changes: 25 additions & 7 deletions modules/core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/blob/LocalBlobStore.java
View file
Expand Up @@ -26,9 +26,9 @@
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.util.stream.Stream;

import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.io.FileUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.nuxeo.ecm.core.api.NuxeoException;
Expand Down Expand Up @@ -224,10 +224,25 @@ public void deleteBlob(String key) {

@Override
public void clear() {
try {
FileUtils.cleanDirectory(pathStrategy.dir.toFile());
// also called when this store is a caching store and GC on the main store was just done
// so we have to avoid deleting tmp files
try (Stream<Path> stream = Files.walk(pathStrategy.dir)) {
stream.forEach(path -> {
if (path == pathStrategy.dir || pathStrategy.isTempFile(path)) {
// don't delete root or tmp files
return;
}
try {
Files.delete(path);
} catch (IOException e) {
if (!Files.isDirectory(path)) {
// don't warn for non-empty dirs
log.warn(e, e);
}
}
});
} catch (IOException e) {
throw new RuntimeException(e);
log.warn(e, e);
}
}

Expand Down Expand Up @@ -306,12 +321,15 @@ protected void deleteOld(File file, long minTime, int depth, boolean delete) {
long lastModified = file.lastModified();
long length = file.length();
if (lastModified == 0) {
log.warn("Cannot read last modified for file: " + file);
log.warn("GC cannot read last modified for file: {}", file);
} else if (lastModified < minTime) {
status.sizeBinariesGC += length;
status.numBinariesGC++;
if (delete && !file.delete()) { // NOSONAR
log.warn("Cannot gc file: " + file);
if (delete) {
log.debug("GC deleting file: {}", file);
if (!file.delete()) { // NOSONAR
log.warn("GC failed to delete file: {}", file);
}
}
} else {
status.sizeBinaries += length;
Expand Down
12 changes: 12 additions & 0 deletions modules/core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/blob/PathStrategy.java
View file
Expand Up @@ -93,6 +93,18 @@ public Path createTempFile() {
}
}

/**
* Checks if the given file is a temporary file
*
* @param path the file
* @return {@code true} if the file is a temporary file
* @since 11.5
*/
public boolean isTempFile(Path path) {
String filename = path.getFileName().toString();
return filename.startsWith("bin_") && filename.endsWith(".tmp");
}

/**
* Gets the storage path for a given key.
*
Expand Down
17 changes: 17 additions & 0 deletions modules/core/nuxeo-core-api/src/test/java/org/nuxeo/ecm/core/blob/TestAbstractBlobStore.java
View file
Expand Up @@ -355,6 +355,11 @@ protected void testCopyOrMove(BlobStore sourceStore, boolean atomicMove) throws
}
}

// overridden by cache tests
protected PathStrategy getCachePathStrategy() {
return null;
}

@Test
public void testGC() throws IOException {
// doesn't bring anything over the LocalBlobStore GC test; avoid additional setup for this
Expand Down Expand Up @@ -402,6 +407,13 @@ public void testGC() throws IOException {
assertBlob(key1, FOO);
assertBlob(key2, "barbaz");

// when caching, create a tmp file in the cache that shouldn't disappear during GC
PathStrategy cachePathStrategy = getCachePathStrategy();
Path cacheTmp = null;
if (cachePathStrategy != null) {
cacheTmp = cachePathStrategy.createTempFile();
}

// real GC
assertFalse(gc.isInProgress());
gc.start();
Expand All @@ -424,6 +436,11 @@ public void testGC() throws IOException {
if (hasGCTimeThreshold()) {
assertBlob(key3, "abcde");
}

// cache tmp is still here
if (cacheTmp != null) {
assertTrue(Files.exists(cacheTmp));
}
}

}
5 changes: 5 additions & 0 deletions modules/core/nuxeo-core-api/src/test/java/org/nuxeo/ecm/core/blob/TestCachingBlobStore.java
View file
Expand Up @@ -23,4 +23,9 @@
@Deploy("org.nuxeo.ecm.core.api.tests:OSGI-INF/test-blob-provider-inmemory-caching.xml")
public class TestCachingBlobStore extends TestAbstractBlobStore {

@Override
protected PathStrategy getCachePathStrategy() {
return ((CachingBlobStore) bs).cacheStore.pathStrategy;
}

}
13 changes: 12 additions & 1 deletion modules/core/nuxeo-core-api/src/test/java/org/nuxeo/ecm/core/blob/TestPathStrategies.java
View file
Expand Up @@ -19,6 +19,7 @@
package org.nuxeo.ecm.core.blob;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import java.io.File;
Expand Down Expand Up @@ -52,7 +53,6 @@ public void testSafeRegex() throws IOException {
assertEquals("%/:\\", rejected);
}


@Test
public void testSafePath() throws IOException {
Path dir = Files.createTempDirectory("tmp_");
Expand Down Expand Up @@ -115,4 +115,15 @@ public void testPathStrategySubDirs() throws IOException {
assertTrue(path, path.endsWith("/%b/ad/%bad%2fkey"));
}

@Test
public void testTempFile() throws IOException {
Path dir = Files.createTempDirectory("tmp_");
PathStrategy ps = new PathStrategyFlat(dir);
Path path = ps.createTempFile();
assertTrue(ps.isTempFile(path));
// non-temp file
Path path2 = path.resolveSibling("1234");
assertFalse(ps.isTempFile(path2));
}

}

0 comments on commit 8d4d5fb

Please sign in to comment.