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.

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

Masai #4

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Masai #4

wants to merge 8 commits into from

Conversation

Raynes
Copy link
Contributor

@Raynes Raynes commented May 18, 2011

I added a Masai external caching algorithm. This gives cache-dot-clj the ability to cache to Redis and Tokyo Cabinet.

I used the ehcache implementation as my base for this. I'm also using all of its non-ehcache-specific tests, and they all pass.

If anything appears out of the ordinary, let me know and I'll fix 'er up! I think this is how it is supposed to work, but I could always be wrong. The fact that the tests pass is probably a good sign though. ;)

@bmabey
Copy link
Collaborator

bmabey commented May 18, 2011

Nice work! It looks good to me. Do you want to merge it in Saul or shall I?

On a side note... With stores like these we should make it clear in the README that this cache is not blocking. Meaning, no locks are put in place which prevent mutliple threads or processes from computing the same value mutiple times simultaneously.

For simple KV stores like this in the past I've created a lock key based on the original key and it tends to work reasonably well. If we do want to add support for this then we would want to modify the external cache mechanism in store and have each strategy provide a lock and unlock functions as well.

@bmabey bmabey closed this May 23, 2011
@bmabey
Copy link
Collaborator

bmabey commented May 23, 2011

I got the go ahead from Sual to be more of a maintainer on this project, so I've merged this in. I plan on making the 0.4.0 release shortly. Thanks again for the patch!

@bmabey bmabey reopened this May 27, 2011
@bmabey
Copy link
Collaborator

bmabey commented May 27, 2011

Hi again... I merged in the patch, but I think it still needs some work. When I tried to run the tests I got:

Exception in thread "main" java.lang.UnsatisfiedLinkError: no jtokyocabinet in java.library.path (tokyo.clj:2)

This isn't surprising since I don't have tokyo cabient installed, much less in my library path. I think this points out a problem though. Currently the masai subproject is requiring redis, tokyo, and db by default:
https://github.com/Raynes/cache-dot-clj/blob/1433e296fb3417209997152813ac6d696fbf928a/masai/src/cache_dot_clj/masai.clj#L3-5

A project that wants to use the masai caching plugin shouldn't have to have all of these unrelated dependencies. Could you make the patch lazily load the appropriate masai namespace based on what is selected by the strategy? (i.e. Move the require down to the strategy definition.)

Also, making jedis, etc a dependency of this project may not be the best option:
https://github.com/Raynes/cache-dot-clj/blob/1433e296fb3417209997152813ac6d696fbf928a/masai/project.clj#L4-7
We can add them to the dev-deps for the tests but if someone wanted to use this plugin in a project they will have already declared a dependeny on the store they are using (e.g. jedis) and so adding all the stores provided by masai as deps here will result in needless dep downloading/loading. Taking the approach that masai takes is probably best:
https://github.com/ninjudd/masai/blob/develop/project.clj

@Raynes
Copy link
Contributor Author

Raynes commented May 27, 2011

The db require is a necessity. It's just the Masai protocol definition. However, you're right about where the requires should be and such. Not sure why I didn't do that in the first place. I'll have that done for you by the end of the day. :)

@bmabey
Copy link
Collaborator

bmabey commented May 27, 2011

Cool! BTW, I decided to change the name of the project to clj-cache to be more inline with other clojure project names (e.g. clj-time, clj-file-utils, etc).. and so I could push it to clojars. Anyways, if you want to rebase off my my changes that would be cool: https://github.com/bmabey/clj-cache Otherwise I can update your changes once you are done.

@bmabey
Copy link
Collaborator

bmabey commented May 27, 2011

One more thing... I wasn't able to run the tests but it looks like you may have a bug here:

https://github.com/Raynes/cache-dot-clj/blob/masai/masai/src/cache_dot_clj/masai.clj#L40

Shouldn't s be passed into prefix like so: (prefix f-name s)? (instead of just (prefix f-name))

@Raynes
Copy link
Contributor Author

Raynes commented May 28, 2011

Nice catch. Don't know how it worked with that broken, but it did. :o

I'll make all my changes off of your clj-cache repo and fix this as well. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants