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

Add serialized capacity to Cache_Redis for objects #3181

Closed
wants to merge 1 commit into from

Conversation

chonhan
Copy link

@chonhan chonhan commented Aug 13, 2014

Updated with Tcholakov's suggestion.
Verified on Both Mac and Ubuntu environments and run correctly with my database result.

Updated with Tcholakov's suggestion.
Verified on Both Mac and Ubuntu environments and run correctly with my database result.
@@ -38,6 +38,11 @@
class CI_Cache_redis extends CI_Driver
{
/**
* A key-suffix for distinguishing serialized values.
*/
const KEY_SUFFIX_FOR_SERIALIZATION = '_ci_driver_serialized';
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Remove this constant
  • Hardcode the string to '_ci_redis_serialized:'

On second thought ... it should be a constant, just give it a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have chosen this ugly suffix in order it not to be hit by a developer by accident. I am fine with '_ci_redis_serialized:' too. As I can see below, by this string you mean a prefix.

@narfbg
Copy link
Contributor

narfbg commented Aug 13, 2014

Overall, I see this as something rather hacky ... isn't there a way to separately mark the keys of serialized values? Like creating a list containing all serialized vars? Such a list would be easily read and cached during the library initialization and way easier to work with.

@narfbg
Copy link
Contributor

narfbg commented Aug 13, 2014

Also, I got lost between all those PRs, just commit to one of them.

@ivantcholakov
Copy link
Contributor

Could I answer these questions?

@narfbg
Copy link
Contributor

narfbg commented Aug 13, 2014

Sure, if you know the answers.

@ivantcholakov
Copy link
Contributor

I did not comment the obvious things "Add empty line.", etc. They are to be done.

You figured out a third approach, this is interesting. Let me enumerate the possible ways:

  1. Direct prefixing the serialized value - I abandoned this one, a serialized value may be 10Mb long for example, I don't want to add/check prefix there.
  2. Prefixed keys for serialized values - the current implementation. It has no the disadvantage of 1., it is yet ready and it is tested. I wrote the test and applied driver modifications simultaneously, behavior is visible. Yes code might look "hackish", but it is explainable.
  3. Index of serialized values - your approach. If it is to pass the same test it will look "hackish" I afraid. There is one obstacle to be solved explicitly, the time factor - there are values that expire. At the moment I have no idea how to keep the index synchronized. The idea is interesting, but maybe it is quite challenging, at least for me.

Well, if you like the second approach, you may merge chonhan's contribution as it is now. I promise to polish it after that, according to your comments.

@narfbg
Copy link
Contributor

narfbg commented Aug 13, 2014

Why would it look hackish? All you'll need to do is fetch that list during the library initialization and add to it in save(). Anywhere else you'd just make an in_array() check before (un)serializing ...
The time factor is not an issue. You only care about those lookups after you've fetched the value, so if the item has expired, you'd just return FALSE prior to that. Not that you can't track time of course, it's just not necessary.

@ivantcholakov
Copy link
Contributor

OK, I will try to implement the third approach, slowly, without rush. It makes sense.

@ivantcholakov
Copy link
Contributor

Just in case, before the next try I have polished the second idea by implementing the remarks by @narfbg. Here is the gist: https://gist.github.com/ivantcholakov/e1d889793e469315ec89

@narfbg
Copy link
Contributor

narfbg commented Aug 18, 2014

Implemented in #3186.

@narfbg narfbg closed this Aug 18, 2014
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.

3 participants