From b3a26c3a143401c682a030d2a8622cf27f4cef12 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Tue, 7 Jun 2011 12:57:49 -0700 Subject: [PATCH] Fix the logic that serves file from cache and add unit tests. This closes issue #42. I believe it also closes issue #10, since relative end-dates are no longer handled specially. Change-Id: I9ee259e7ae94a677bc6fc366024ac385a91a1cdb --- Makefile | 1 + src/tsd/GraphHandler.java | 75 +++++++----- src/tsd/TestGraphHandler.java | 215 ++++++++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+), 30 deletions(-) create mode 100644 src/tsd/TestGraphHandler.java diff --git a/Makefile b/Makefile index c5868c2651..021777f18e 100644 --- a/Makefile +++ b/Makefile @@ -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 \ diff --git a/src/tsd/GraphHandler.java b/src/tsd/GraphHandler.java index 91b7359764..72a8c208f7 100644 --- a/src/tsd/GraphHandler.java +++ b/src/tsd/GraphHandler.java @@ -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; @@ -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. @@ -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") @@ -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\":"); @@ -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")) { @@ -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; } @@ -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. @@ -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); diff --git a/src/tsd/TestGraphHandler.java b/src/tsd/TestGraphHandler.java new file mode 100644 index 0000000000..79d0ca7573 --- /dev/null +++ b/src/tsd/TestGraphHandler.java @@ -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 . +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.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; + } + +}