Added generic Resource caching capabilities #183

lucaspcram · 2018-07-31T18:44:41Z

EDIT: Ready for merge

This PR adds extensible Resource caching capabilities to the atlas.

The basic idea is that users can create a cache with a default population mechanism, choose a caching strategy, and then fetch desired resources (URIs) using the strategy. A very simple use case might look like:

// Let's read a resource at an arbitrary URI into a ByteArrayResource
// notice the function provided to withFetcher conforms to the ResourceFetchFunction functional interface

final URI LOCAL_TEST_FILE_URI = URI.create("file:///path/to/some/file.txt");

// cache file contents into memory (a byte array)
final ConcurrentResourceCache resourceCache = new ConcurrentResourceCache(new ByteArrayCachingStrategy(), uri -> new File(uri.getPath()));

// this will cache miss the first time and read the file from disk (using the provided fetcher)
final ByteArrayResource fileBytes = new ByteArrayResource();
fileBytes.copyFrom(resourceCache.get(LOCAL_TEST_FILE_URI).get());

// this time we hit the cache, and read the bytes from memory instead of from disk
final ByteArrayResource fileBytesAgain = new ByteArrayResource();
fileBytesAgain.copyFrom(resourceCache.get(LOCAL_TEST_FILE_URI).get());



// Manually setting fetchers and CachingStrategies can be a pain.
// You can abstract all this away by creating case-specific subclasses.
// This subclass cache uses ByteArrayCachingStrategy by default and
// abstracts the get URI parameter by just taking a path string as a parameter instead.
final LocalFileInMemoryCache fileCache = new LocalFileInMemoryCache();

final ByteArrayResource fileBytes3 = new ByteArrayResource();
fileBytes3.copyFrom(fileCache.get("/path/to/another/file.txt").get());

See class CachingTests for test cases that demonstrate in detail how the caches can be used.

In an upcoming PR to AtlasGenerator, I will add a subclass of ResourceCache with an interface designed specifically for caching files with structured URIs from remote blob stores. See LocalFileInMemoryCache for an example of how to modify the interface of ResourceCache.

jklamer

Haven't finished reading yet but

lucaspcram · 2018-08-01T16:12:25Z

Haha @jklamer does this look like something you might find useful?

matthieun

The code looks great, thanks @lucaspcram, and this will be very useful! Things to add to my mind:

More tests
- Including integration tests to make sure the local file system storage is deterministic
Support for concurrency
A short Readme.md file that has a basic explanation and a couple code snippets.

matthieun · 2018-08-01T17:12:59Z

src/main/java/org/openstreetmap/atlas/utilities/caching/ResourceFetchFunction.java

+ * @author lcram
+ */
+@FunctionalInterface
+public interface ResourceFetchFunction


Instead of having an interface for that, could this just be a java.util.function.Function<URI, Resource>?

matthieun · 2018-08-01T17:15:14Z

...org/openstreetmap/atlas/utilities/caching/strategies/SystemTemporaryFileCachingStrategy.java

+    {
+        if (!cachedFile.exists())
+        {
+            logger.info("Attempting to cache resource {} in temporary file {}", resourceURI,


This being a fairly low level operation, I would log it as debug or even trace, and only bubble up the errors as warn or error.

snb3300

Nice @lucaspcram 👍 Few suggestion inline

snb3300 · 2018-08-01T17:31:41Z

src/main/java/org/openstreetmap/atlas/utilities/caching/ResourceFetchFunction.java

+ * @author lcram
+ */
+@FunctionalInterface
+public interface ResourceFetchFunction


Instead of creating a separate interface why not use the default Function interface Function<URI, Resource>

Oops, I guess @matthieun suggested the same

snb3300 · 2018-08-01T17:33:57Z

src/main/java/org/openstreetmap/atlas/utilities/caching/ResourceCache.java

+
+    private URI resourceURI;
+    private CachingStrategy cachingStrategy;
+    private ResourceFetchFunction defaultFetcher;


type of defaultFetcher can be Function<URI, Resource> instead of defining your own type. What do you think about it?

Good idea. @matthieun suggested this as well.

snb3300 · 2018-08-01T17:36:09Z

src/main/java/org/openstreetmap/atlas/utilities/caching/LocalFileInMemoryCache.java

+     */
+    public LocalFileInMemoryCache(final URI resourceURI)
+    {
+        super(resourceURI, new ByteArrayCachingStrategy(), null);


Any reason you are setting null defaultFetcher in the super call and then using the setter?

I think this was just an oversight on my part. Good catch.

…log levels.

MikeGost

Great code, thanks @lucaspcram! +1 for multi-threaded support.

MikeGost · 2018-08-01T18:13:32Z

src/main/java/org/openstreetmap/atlas/utilities/caching/ResourceCache.java

+
+/**
+ * The basic resource cache implementation. This cache loads a resource using a given {@link URI},
+ * {@link CachingStrategy}, and default {@link ResourceFetchFunction}. Since using raw URIs can


Nit: ResourceFetchFunction no longer exists.

MikeGost · 2018-08-01T18:14:01Z

src/main/java/org/openstreetmap/atlas/utilities/caching/ResourceCache.java

+
+    private URI resourceURI;
+    private CachingStrategy cachingStrategy;
+    private Function<URI, Resource> defaultFetcher;


Nit: why default fetcher vs. fetcher?

MikeGost · 2018-08-01T18:16:22Z

src/main/java/org/openstreetmap/atlas/utilities/caching/ResourceCache.java

+     */
+    private Resource getResourceDirectly()
+    {
+        if (this.defaultFetcher == null)


We do a couple of checks for fetcher being null. To follow the pattern you introduced for checking to see if URI was null, does it make sense to wrap this in a private method as well? Something like throwCoreExceptionIfResourceFetcherIsNull

MikeGost · 2018-08-01T18:19:18Z

...org/openstreetmap/atlas/utilities/caching/strategies/SystemTemporaryFileCachingStrategy.java

+            logger.trace("Cache hit on resource {}, returning local copy", resourceURI);
+            return Optional.of(cachedFile);
+        }
+        else


Nit: can get rid of else here and just return Optional.empty() with the logging above.

MikeGost · 2018-08-01T20:22:12Z

Talking offline - Guava Cache as a backend (handles thread safety) might be a option to consider.

lucaspcram · 2018-08-06T21:46:37Z

This PR is ready for merge.

MikeGost

Nicely done, clean and tested! Thanks @lucaspcram!

MikeGost · 2018-08-07T18:00:50Z

...org/openstreetmap/atlas/utilities/caching/strategies/SystemTemporaryFileCachingStrategy.java

+                }
+                catch (final Exception exception)
+                {
+                    logger.error("Something went wrong moving {} to {}",


Let's add the exception as the last parameter to the logger call, so we don't lose the stack trace if this ever happens.

MikeGost · 2018-08-07T18:01:16Z

...org/openstreetmap/atlas/utilities/caching/strategies/SystemTemporaryFileCachingStrategy.java

+            }
+            catch (final Exception exception)
+            {
+                logger.error("Something went wrong copying {} to temporary local file {}",


Same comment here.

MikeGost · 2018-08-07T18:02:25Z

src/test/java/org/openstreetmap/atlas/utilities/caching/CachingTests.java

+/**
+ * @author lcram
+ */
+public class CachingTests


Nice tests!

MikeGost · 2018-08-07T18:03:18Z

...org/openstreetmap/atlas/utilities/caching/strategies/SystemTemporaryFileCachingStrategy.java

+        throw new UnsupportedOperationException("Operation not supported at this time.");
+    }
+
+    private void attemptToCacheFileLocally(final File cachedFile,


If this code was copy/pasted from existing caching file, make sure to move author tags as well.

It's loosely based off of part my version of fetchAtlas from AtlasGeneratorHelper, so I will add both your and Matthieu's author tags to this one.

MikeGost · 2018-08-13T17:27:49Z

Great work here, @lucaspcram! Merging.

lucaspcram added 7 commits July 30, 2018 15:27

Working on prototype of abstract resource caching

27899f2

Slight refactor

c8ccf54

More refactoring

48bfa73

Totally new design

eacb789

Added tests

525ef77

Added even more testing

8802f0d

Checkstyle

ec183dc

lucaspcram added the New Feature label Jul 31, 2018

lucaspcram requested review from matthieun, jwpgage and MikeGost July 31, 2018 18:44

lucaspcram added 3 commits July 31, 2018 14:42

Refactored comments and tests

8741d3c

Added forced defaults to the LocalFileInMemoryCache

21b7266

Refactored local file cache again.

9607f56

jklamer reviewed Aug 1, 2018

View reviewed changes

Changed test names

002dab2

Added a URI string shortcut to ResourceCache

e33e276

lucaspcram changed the title ~~Added generic Resource caching capabilities~~ DNM: Added generic Resource caching capabilities Aug 1, 2018

matthieun requested changes Aug 1, 2018

View reviewed changes

snb3300 approved these changes Aug 1, 2018

View reviewed changes

Removed unnecessary ResourceFetchFunction interface and changed some …

fa6e006

…log levels.

MikeGost approved these changes Aug 1, 2018

View reviewed changes

Addressed more PR comments

3dad8ac

lucaspcram added 5 commits August 3, 2018 08:59

Refactored to make threadsafe. Need to add tests.

93534fa

Added a Cache interface

b8f38e6

Added javadoc

a3fc6f7

Fixed dumb compiler and checkstyle errors

1865c13

More test updates

08c4580

lucaspcram added 5 commits August 3, 2018 11:27

Added a large test to attempt to see if multithreaded is working

f639f95

go away checkstyle

3675bac

Added README

562acdf

Updated caching test

5c3fb4c

Added an invalidate to the CachingStrategy interface

40f708d

lucaspcram changed the title ~~DNM: Added generic Resource caching capabilities~~ Added generic Resource caching capabilities Aug 6, 2018

Changed a document word

cacdcf1

MikeGost approved these changes Aug 7, 2018

View reviewed changes

MikeGost reviewed Aug 7, 2018

View reviewed changes

Addressed PR comments.

4fc80f9

MikeGost merged commit a2764a0 into osmlab:dev Aug 13, 2018

lucaspcram deleted the atlascache branch August 14, 2018 17:38

lucaspcram added this to the 5.1.8 milestone Aug 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added generic Resource caching capabilities #183

Added generic Resource caching capabilities #183

lucaspcram commented Jul 31, 2018 •

edited

Loading

jklamer left a comment

lucaspcram commented Aug 1, 2018

matthieun left a comment

matthieun Aug 1, 2018

matthieun Aug 1, 2018

snb3300 left a comment

snb3300 Aug 1, 2018

snb3300 Aug 1, 2018

snb3300 Aug 1, 2018

lucaspcram Aug 1, 2018

snb3300 Aug 1, 2018

lucaspcram Aug 1, 2018

MikeGost left a comment

MikeGost Aug 1, 2018

MikeGost Aug 1, 2018

MikeGost Aug 1, 2018

MikeGost Aug 1, 2018

MikeGost commented Aug 1, 2018

lucaspcram commented Aug 6, 2018

MikeGost left a comment

MikeGost Aug 7, 2018

MikeGost Aug 7, 2018

MikeGost Aug 7, 2018

MikeGost Aug 7, 2018

lucaspcram Aug 10, 2018 •

edited

Loading

MikeGost commented Aug 13, 2018

Added generic Resource caching capabilities #183

Added generic Resource caching capabilities #183

Conversation

lucaspcram commented Jul 31, 2018 • edited Loading

jklamer left a comment

Choose a reason for hiding this comment

lucaspcram commented Aug 1, 2018

matthieun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snb3300 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeGost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeGost commented Aug 1, 2018

lucaspcram commented Aug 6, 2018

MikeGost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaspcram Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

MikeGost commented Aug 13, 2018

lucaspcram commented Jul 31, 2018 •

edited

Loading

lucaspcram Aug 10, 2018 •

edited

Loading