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

Cache TTL should be a Duration instead of Time #100

Merged
merged 6 commits into from
Jun 26, 2013
Merged

Cache TTL should be a Duration instead of Time #100

merged 6 commits into from
Jun 26, 2013

Conversation

ximyu
Copy link
Contributor

@ximyu ximyu commented May 1, 2013

No description provided.

@@ -54,11 +54,11 @@ class MemcacheStore(val client: Client, ttl: Time, flag: Int) extends Store[Stri
.mapValues { _.flatten }
}

protected def set(k: String, v: ChannelBuffer) = client.set(k, flag, ttl, v)
protected def set(k: String, v: ChannelBuffer) = client.set(k, flag, ttl.fromNow, v)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh boy, this is a huge bug if that's the real meaning of the API. Can you link me to a doc?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be sure about this before merge.

@ximyu
Copy link
Contributor Author

ximyu commented May 1, 2013

I will ping you in IM as I need to share some internal links.

@softprops
Copy link
Contributor

Many of the redis interfaces ( initially based on the memcache interface ) also use time I now see how this can be a problem for long lived stores.

@ximyu
Copy link
Contributor Author

ximyu commented May 1, 2013

@softprops yep. With current interface we'll have to instantiate a Store instance on every attempt to do set.

@ximyu
Copy link
Contributor Author

ximyu commented May 1, 2013

Chatted with the owner of finagle-memcached. Seems like finagle-memcached allows two types of semantics when setting cache TTL and the existing Storehaus code is correct. I'll close this pull request.

@ximyu ximyu closed this May 1, 2013
@sritchie
Copy link
Collaborator

sritchie commented May 1, 2013

I do still prefer the Duration vs Time switch! I think it makes more sense and is more in line with the rest of the code we're writing.

@ximyu ximyu reopened this May 1, 2013
@softprops
Copy link
Contributor

I agree with @sritchie

@rubanm
Copy link
Contributor

rubanm commented May 2, 2013

+1

@ximyu
Copy link
Contributor Author

ximyu commented May 7, 2013

@johnynek @sritchie just wondering if any of you could help me take a look at this updated pull request? Thanks.

@sritchie
Copy link
Collaborator

Looks like this req needs a merge. @johnynek, can you take a look at the new TTL trait?

@softprops
Copy link
Contributor

Not sure is if this is worth nitpicking over, but does WithPutTTL makes more sense? TTL is acronym, like URL.

@johnynek
Copy link
Collaborator

I prefer the Ttl style because it is slightly easier to type. There does not seem to be a standard:

http://stackoverflow.com/questions/5704737/java-naming-convention-with-acronyms

* Trait for building mutable store with TTL.
*/
trait WithPutTtl[K, V, S <: Store[K, V]] {
def withTtl(ttl: Duration): S
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be safer to make it withPutTtl in case there are multiple TTLs (note the trait has the Put).

@johnynek
Copy link
Collaborator

Looks good te me. Let's get a link to the docs about the question Sam asked (ttl.fromNow).

@ximyu
Copy link
Contributor Author

ximyu commented May 23, 2013

Renamed to withPutTtl and also added documentation links to Memcached and Redis about setting expiration time.

@sritchie
Copy link
Collaborator

@ximyu, can you merge with Develop? Then let's get this in.

@sritchie
Copy link
Collaborator

ping!

@ximyu
Copy link
Contributor Author

ximyu commented Jun 18, 2013

Oops, I'll get this merged this week :)

@sritchie
Copy link
Collaborator

@ximyu that'd be awesome! Can't wait.

@johnynek
Copy link
Collaborator

ping!!!!!!!!!!!!!!!!

…evelop

Conflicts:
	storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisLongStore.scala
	storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisStringStore.scala
ximyu added a commit that referenced this pull request Jun 26, 2013
Cache TTL should be a Duration instead of Time
@ximyu ximyu merged commit 5b1bf12 into twitter:develop Jun 26, 2013
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.

5 participants