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

Feature bit cache warming #20

Merged
merged 1 commit into from
Dec 29, 2011
Merged

Conversation

kbuckler
Copy link

As discussed. A cache miss may cause all feature bits to be preloaded, up to Arturo.cache_warming_limit.

A cache miss that does not find the requested feature bit in the database will not cause this to happen. This is intentional.

@jamesarosen
Copy link
Contributor

Doesn't this warm the cache every time the cache misses but the database has the record, not just the first time?

@kbuckler
Copy link
Author

Yeah, that's right though something may have gotten lost in translation from my other pull request. Each cache miss + database hit preloads all other records.

I suppose this might be a problem in some environments, where the cache isn't in-process.

@jamesarosen
Copy link
Contributor

Either I'm totally misreading this or you're missing my point.

  1. User requests a feature, foo
  2. feature_cache_warming_enabled returns true
  3. warm_feature_cache loads the most recent 150 records
  4. but foo isn't one of them, so it's never written to the cache
  5. later, another user requests foo
  6. we again load the 150 most recent features, and foo still isn't among them, so it isn't cached

@kbuckler
Copy link
Author

Nope, you didn't misread, that's actually a good point. That will happen when foo is older than cache_warming_limit other records. But my earlier statement still stands, that each cache miss and database hit sequence will preload all other features, up to the configured limit.

I'm thinking that the cache_warming_limit isn't necessary, and might even be problemsome in other ways. Chances are if we hit that in the future we'll just increase it and move on without second thought.

@kbuckler
Copy link
Author

Squashed.

jamesarosen added a commit that referenced this pull request Dec 29, 2011
@jamesarosen jamesarosen merged commit d686447 into zendesk:rails_2_3 Dec 29, 2011
@jamesarosen
Copy link
Contributor

Now we need to port this to the main branch...

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