Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NXP-30271: keep tmp files during key/value store GC #4722

Merged
merged 1 commit into from Mar 24, 2021
Merged

NXP-30271: keep tmp files during key/value store GC #4722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 47 additions & 10 deletions modules/core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/blob/CachingBlobStore.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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));
}

}