Closed dazza-codes closed 9 years ago
Note that the suggested solution requires additional gems in the gemspec, which are not included in this pull request (which was auto-generated by the github UI after editing engine.rb
online at github).
What made you decide to put this code in engine.rb?
(Probably in future, better to edit in a local branch and push after tests pass)
@ndushay re config in engine.rb...
See http://edgeguides.rubyonrails.org/engines.html#inside-an-engine, which identifies and discusses the lib/blorgh/engine.rb
file and states, "Some engines choose to use this file to put global configuration options for their engine. It's a relatively good idea, so if you want to offer configuration options, the file where your engine's module is defined is perfect for that. Place the methods inside the module and you'll be good to go."
Although this pull request contains a quick 'template' cut-n-paste, so it has not yet created methods in the module and it should do so. See also http://brandonhilkert.com/blog/ruby-gem-configuration-patterns/
Caching should be an application concern, and if triannon wants to use it, it could generate appropriate configuration into the application.
@cbeer might be confusing app-cache with rest-client cache. This issue is about retrieving json-ld context docs from external resources and caching the results in the engine. This is not about caching results returned in a response from triannon. HTH to clarify.
@darrenleeweber yes, I get that. I don't think triannon should care that we're caching retrieved data in /tmp/cache
(or anywhere else...), that's an application's concern (and one that we should strongly encourage, of course.).
Note that rack-cache
has a variety of cache backends (memcache and file out-of-the-box), and we probably should let triannon implementations decide what cache they want to use (instead of inflicting our preferences on them.)
@cbeer I see your point, but:
If we don't have the triannon gem itself cache contexts, then what would you suggest we do to cache jsonld contexts for our specs? And for spinning up triannon via engine_cart for development purposes? We've been using vcr as a poor man's testing cache, but implementing search implies there will be a lot more of these lookups (1 lookup per anno, per test), and that's kind of a ridiculous approach anyway. Do you envision us mocking the cache lookups in spec helper? Add configurable caching to the gem and have it set up for the test and dev environments but not for prod?
All I really want to do, for heavens sake, is use a locally stored cache for the 3 possible context urls when reading jsonld into a graph object. This app will go to those urls for every jsonld read. I already spent some hours combing over the RDF code trying to figure out a simple way to do this. Plus some time pouring over rack cache to see if I could turn it on ONLY for these docs, as the management of a local cache of any size becomes an additional headache. With Willy's help, I concluded "hey, let's try Darren's rack cache solution and see what ends up in the cache" as other solutions weren't trivial, at least to me. Darren surprised himself and me with this branch creation; I'm working locally on my laptop to see if I can get it working. Your input is very welcome ... but this is but one step in my implementing the search API for triannon, which is one of about 10 tasks in getting triannon to beta. I may need more help than a few comments in this issue.
I freely admit I'm not a knowledgeable rails developer. My task is to bring this project to beta (not production, mind you, but beta); your suggestions are really helpful but there is a tradeoff between my current throughput and "how we think it should be done" given that we as yet have no other triannon adopters. Planning for more configurable caching: very good; making it harder to implement: not great; stopping for every bump in the road to "fix it right": not practical without more help, and not really alpha-level development (I haven't even got search via triannon api working yet; I am anticipating that w3c would lock me out if I working on implementing search without context caching). "fix it right" may be "get it working now" for this first implementation.
The truth is, this shouldn't have been a branch yet, and certainly not a pull request. I think Darren was noodling using github's editor. Essentially, he did a cut-and-paste from the issue ticket into the engine.rb file. I'm not sure why he tried that, but I don't think that will happen again. I will do a separate pull request when I have cache code working. There will be more code there allowing for more line comments about how to do it better.
Again, I'm not saying we shouldn't be caching. I am saying that how we cache should be entirely within the application's control. One good pattern for doing this in Rails is to create a generator that adds an initializer into the host application.
Here's an example of what I'm trying to suggest: https://github.com/sul-dlss/triannon/compare/rest-client-caching-generator (too bad rest-client-components
isn't compatible with the latest rest-client..) I'm seeing a couple failures locally, but they don't seem to have anything to do with rest-client
, so I pushed it up to see what travis thinks.
The nice thing about using the generator is the cache creation ends up within your host application, so if I want to use memcached or something else, I could easily do that (and, if I wanted to do something different in different environments, that's a concern for my application, not code you have to maintain in triannon).
The cut-n-paste code that is working OK in the annotations2triannon gem uses ENV values to determine the cache parameters, e.g. https://github.com/sul-dlss/annotations2triannon/blob/master/.env_example#L19-L26
The meta-store should be high performance (heap for small data / memcache for larger data). The entity-store probably should not be heap, although that's the default. To avoid the default rack-cache in the heap, defaults can be set to files, e.g. https://github.com/sul-dlss/annotations2triannon/blob/master/lib/requires.rb#L10-L36
Potential solution for context caching improvement #96