Skip to content

Commit

Permalink
Fix the logic that serves file from cache and add unit tests.
Browse files Browse the repository at this point in the history
This closes issue OpenTSDB#42.  I believe it also closes issue OpenTSDB#10, since
relative end-dates are no longer handled specially.

Change-Id: I9ee259e7ae94a677bc6fc366024ac385a91a1cdb
  • Loading branch information
tsuna committed Jun 7, 2011
1 parent 33dff14 commit b3a26c3
Show file tree
Hide file tree
Showing 3 changed files with 261 additions and 30 deletions.
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ tsdb_LIBADD = \
test_JAVA = \
src/core/TestTags.java \
src/stats/TestHistogram.java \
src/tsd/TestGraphHandler.java \
src/uid/TestNoSuchUniqueId.java \
src/uid/TestUniqueId.java \

Expand Down
75 changes: 45 additions & 30 deletions src/tsd/GraphHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ private void doGraph(final TSDB tsdb, final HttpQuery query)
final int max_age = (end_time > now ? 0 // (1)
: (end_time < now - Const.MAX_TIMESPAN ? 86400 // (2)
: (int) (end_time - start_time) >> 10)); // (3)
if (!nocache && isDiskCacheHit(query, max_age, basepath)) {
if (!nocache && isDiskCacheHit(query, end_time, max_age, basepath)) {
return;
}
Query[] tsdbqueries;
Expand Down Expand Up @@ -321,6 +321,7 @@ private String getGnuplotBasePath(final HttpQuery query) {
/**
* Checks whether or not it's possible to re-serve this query from disk.
* @param query The query to serve.
* @param end_time The end time on the query (32-bit unsigned int, seconds).
* @param max_age The maximum time (in seconds) we wanna allow clients to
* cache the result in case of a cache hit.
* @param basepath The base path used for the Gnuplot files.
Expand All @@ -329,6 +330,7 @@ private String getGnuplotBasePath(final HttpQuery query) {
* the query needs to be processed).
*/
private boolean isDiskCacheHit(final HttpQuery query,
final long end_time,
final int max_age,
final String basepath) throws IOException {
final String cachepath = basepath + (query.hasQueryStringParam("ascii")
Expand All @@ -342,11 +344,11 @@ private boolean isDiskCacheHit(final HttpQuery query,
+ bytes + " bytes) to be valid. Ignoring it.");
return false;
}
if (staleCacheFile(query, max_age, cachedfile)) {
if (staleCacheFile(query, end_time, max_age, cachedfile)) {
return false;
}
if (query.hasQueryStringParam("json")) {
StringBuilder json = loadCachedJson(query, max_age, basepath);
StringBuilder json = loadCachedJson(query, end_time, max_age, basepath);
if (json == null) {
json = new StringBuilder(32);
json.append("{\"timing\":");
Expand All @@ -367,7 +369,7 @@ private boolean isDiskCacheHit(final HttpQuery query,
}
// We didn't find an image. Do a negative cache check. If we've seen
// this query before but there was no result, we at least wrote the JSON.
final StringBuilder json = loadCachedJson(query, max_age, basepath);
final StringBuilder json = loadCachedJson(query, end_time, max_age, basepath);
// If we don't have a JSON file it's a complete cache miss. If we have
// one, and it says 0 data points were plotted, it's a negative cache hit.
if (json == null || !json.toString().contains("\"plotted\":0")) {
Expand All @@ -391,38 +393,49 @@ private boolean isDiskCacheHit(final HttpQuery query,
/**
* Returns whether or not the given cache file can be used or is stale.
* @param query The query to serve.
* @param end_time The end time on the query (32-bit unsigned int, seconds).
* @param max_age The maximum time (in seconds) we wanna allow clients to
* cache the result in case of a cache hit.
* cache the result in case of a cache hit. If the file is exactly that
* old, it is not considered stale.
* @param cachedfile The file to check for staleness.
*/
private boolean staleCacheFile(final HttpQuery query,
final long max_age,
final File cachedfile) {
private static boolean staleCacheFile(final HttpQuery query,
final long end_time,
final long max_age,
final File cachedfile) {
final long mtime = cachedfile.lastModified() / 1000;
if (mtime <= 0) {
return true; // File doesn't exist, or can't be read.
}
// Queries that don't specify an end-time must be handled carefully,
// since time passes and we may need to regenerate the results in case
// new data points have arrived in the mean time.
final String end = query.getQueryStringParam("end");
if (end == null || end.endsWith("ago")) {
// How many seconds stale?
final long staleness = System.currentTimeMillis() / 1000 - mtime;
if (staleness < 0) { // Can happen if the mtime is "in the future".
logWarn(query, "Not using file @ " + cachedfile + " with weird"
+ " mtime in the future: " + mtime);
return true;
}
// If our graph is older than 0.1% of the duration of the request,
// let's regenerate it in order to avoid service data that's too
// stale, e.g., for 1h of data, it's OK to serve something 3s stale.
if (staleness > max_age) {
logInfo(query, "Cached file @ " + cachedfile.getPath() + " is "
+ staleness + "s stale, which is more than its limit of "
+ max_age + "s, and needs to be regenerated.");
return true;
}

final long now = System.currentTimeMillis() / 1000;
// How old is the cached file, in seconds?
final long staleness = now - mtime;
if (staleness < 0) { // Can happen if the mtime is "in the future".
logWarn(query, "Not using file @ " + cachedfile + " with weird"
+ " mtime in the future: " + mtime);
return true; // Play it safe, pretend we can't use this file.
}

// Case 1: The end time is an absolute point in the past.
// We might be able to re-use the cached file.
if (0 < end_time && end_time < now) {
// If the file was created prior to the end time, maybe we first
// executed this query while the result was uncacheable. We can
// tell by looking at the mtime on the file. If the file was created
// before the query end time, then it contains partial results that
// shouldn't be served again.
return mtime < end_time;
}

// Case 2: The end time of the query is now or in the future.
// The cached file contains partial data and can only be re-used if it's
// not too old.
if (staleness > max_age) {
logInfo(query, "Cached file @ " + cachedfile.getPath() + " is "
+ staleness + "s stale, which is more than its limit of "
+ max_age + "s, and needs to be regenerated.");
return true;
}
return false;
}
Expand Down Expand Up @@ -496,6 +509,7 @@ private static byte[] readFile(final HttpQuery query,
/**
* Attempts to read the cached {@code .json} file for this query.
* @param query The query to serve.
* @param end_time The end time on the query (32-bit unsigned int, seconds).
* @param max_age The maximum time (in seconds) we wanna allow clients to
* cache the result in case of a cache hit.
* @param basepath The base path used for the Gnuplot files.
Expand All @@ -505,11 +519,12 @@ private static byte[] readFile(final HttpQuery query,
* the time taken to serve by the request and other JSON elements if wanted.
*/
private StringBuilder loadCachedJson(final HttpQuery query,
final long end_time,
final long max_age,
final String basepath) {
final String json_path = basepath + ".json";
File json_cache = new File(json_path);
if (staleCacheFile(query, max_age, json_cache)) {
if (staleCacheFile(query, end_time, max_age, json_cache)) {
return null;
}
final byte[] json = readFile(query, json_cache, 4096);
Expand Down
215 changes: 215 additions & 0 deletions src/tsd/TestGraphHandler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
// This file is part of OpenTSDB.
// Copyright (C) 2011 StumbleUpon, Inc.
//
// This program is free software: you can redistribute it and/or modify it
// under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or (at your
// option) any later version. This program is distributed in the hope that it
// will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty
// of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser
// General Public License for more details. You should have received a copy
// of the GNU Lesser General Public License along with this program. If not,
// see <http://www.gnu.org/licenses/>.
package net.opentsdb.tsd;

import java.io.File;
import java.text.SimpleDateFormat;
import java.util.Date;

import org.jboss.netty.channel.Channel;

import org.junit.Test;
import org.junit.runner.RunWith;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import org.mockito.ArgumentMatcher;
import org.mockito.InOrder;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.argThat;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import org.powermock.api.mockito.PowerMockito;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;
import org.powermock.reflect.Whitebox;
import static org.powermock.api.mockito.PowerMockito.mock;

@RunWith(PowerMockRunner.class)
@PrepareForTest({ GraphHandler.class, HttpQuery.class })
public final class TestGraphHandler {

@Test // If the file doesn't exist, we don't use it, obviously.
public void staleCacheFileDoesntExist() throws Exception {
final File cachedfile = fakeFile("/cache/fake-file");
// From the JDK manual: "returns 0L if the file does not exist
// or if an I/O error occurs"
when(cachedfile.lastModified()).thenReturn(0L);

assertTrue("File is stale", staleCacheFile(null, 0, 10, cachedfile));

verify(cachedfile).lastModified(); // Ensure we do a single stat() call.
}

@Test // If the mtime of a file is in the future, we don't use it.
public void staleCacheFileInTheFuture() throws Exception {
PowerMockito.mockStatic(System.class);

final HttpQuery query = fakeHttpQuery();
final File cachedfile = fakeFile("/cache/fake-file");

final long now = 1000L;
when(System.currentTimeMillis()).thenReturn(now);
when(cachedfile.lastModified()).thenReturn(now + 1000L);
final long end_time = now;

assertTrue("File is stale",
staleCacheFile(query, end_time, 10, cachedfile));

verify(cachedfile).lastModified(); // Ensure we do a single stat() call.
PowerMockito.verifyStatic(); // Verify that ...
System.currentTimeMillis(); // ... this was called only once.
}

@Test // End time in the future => OK to serve stale file up to max_age.
public void staleCacheFileEndTimeInFuture() throws Exception {
PowerMockito.mockStatic(System.class);

final HttpQuery query = fakeHttpQuery();
final File cachedfile = fakeFile("/cache/fake-file");

final long end_time = 20000L;
when(System.currentTimeMillis()).thenReturn(10000L);
when(cachedfile.lastModified()).thenReturn(8000L);

assertFalse("File is not more than 3s stale",
staleCacheFile(query, end_time, 3, cachedfile));
assertFalse("File is more than 2s stale",
staleCacheFile(query, end_time, 2, cachedfile));
assertTrue("File is more than 1s stale",
staleCacheFile(query, end_time, 1, cachedfile));

// Ensure that we stat() the file and look at the current time once per
// invocation of staleCacheFile().
verify(cachedfile, times(3)).lastModified();
PowerMockito.verifyStatic(times(3));
System.currentTimeMillis();
}

@Test // No end time = end time is now.
public void staleCacheFileEndTimeIsNow() throws Exception {
PowerMockito.mockStatic(System.class);

final HttpQuery query = fakeHttpQuery();
final File cachedfile = fakeFile("/cache/fake-file");

final long now = 10000L;
final long end_time = now;
when(System.currentTimeMillis()).thenReturn(now);
when(cachedfile.lastModified()).thenReturn(8000L);

assertFalse("File is not more than 3s stale",
staleCacheFile(query, end_time, 3, cachedfile));
assertFalse("File is more than 2s stale",
staleCacheFile(query, end_time, 2, cachedfile));
assertTrue("File is more than 1s stale",
staleCacheFile(query, end_time, 1, cachedfile));

// Ensure that we stat() the file and look at the current time once per
// invocation of staleCacheFile().
verify(cachedfile, times(3)).lastModified();
PowerMockito.verifyStatic(times(3));
System.currentTimeMillis();
}

@Test // End time in the past, file's mtime predates it.
public void staleCacheFileEndTimeInPastOlderFile() throws Exception {
PowerMockito.mockStatic(System.class);

final HttpQuery query = fakeHttpQuery();
final File cachedfile = fakeFile("/cache/fake-file");

final long end_time = 8000L;
final long now = end_time + 2000L;
when(System.currentTimeMillis()).thenReturn(now);
when(cachedfile.lastModified()).thenReturn(5000L);

assertTrue("File predates end-time and cannot be re-used",
staleCacheFile(query, end_time, 4, cachedfile));

verify(cachedfile).lastModified(); // Ensure we do a single stat() call.
PowerMockito.verifyStatic(); // Verify that ...
System.currentTimeMillis(); // ... this was called only once.
}

@Test // End time in the past, file's mtime is after it.
public void staleCacheFileEndTimeInPastCacheableFile() throws Exception {
PowerMockito.mockStatic(System.class);

final HttpQuery query = fakeHttpQuery();
final File cachedfile = fakeFile("/cache/fake-file");

final long end_time = 8000L;
final long now = end_time + 2000L;
when(System.currentTimeMillis()).thenReturn(now);
when(cachedfile.lastModified()).thenReturn(end_time + 1000L);

assertFalse("File was created after end-time and can be re-used",
staleCacheFile(query, end_time, 1, cachedfile));

verify(cachedfile).lastModified(); // Ensure we do a single stat() call.
PowerMockito.verifyStatic(); // Verify that ...
System.currentTimeMillis(); // ... this was called only once.
}

private static String mktime(final long millis) {
final SimpleDateFormat fmt = new SimpleDateFormat("yyyy/MM/dd-HH:mm:ss");
return fmt.format(new Date(millis));
}

/**
* Helper to call private static method.
* There's one slight difference: the {@code end_time} parameter is in
* milliseconds here, instead of seconds.
*/
private static boolean staleCacheFile(final HttpQuery query,
final long end_time,
final long max_age,
final File cachedfile) throws Exception {
return Whitebox.<Boolean>invokeMethod(GraphHandler.class, "staleCacheFile",
query, end_time / 1000, max_age,
cachedfile);
}

private static HttpQuery fakeHttpQuery() {
final HttpQuery query = mock(HttpQuery.class);
final Channel chan = fakeChannel();
when(query.channel()).thenReturn(chan);
return query;
}

private static Channel fakeChannel() {
final Channel chan = mock(Channel.class);
when(chan.toString()).thenReturn("[fake channel]");
return chan;
}

private static File fakeFile(final String path) {
final File file = mock(File.class);
when(file.getPath()).thenReturn(path);
when(file.toString()).thenReturn(path);
return file;
}

}

0 comments on commit b3a26c3

Please sign in to comment.