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

New option for Modis::Model - :enable_all_index #7

Merged
merged 2 commits into from
Jun 23, 2017

Conversation

nattfodd
Copy link
Contributor

@nattfodd nattfodd commented Jun 12, 2017

Allows to disable creating of my:core:key:all kind of keys if they are not needed.
Getting rid of these keys is important feature, because those keys consume a huge amount of memory.

For example, one single *:all key having 158m ids consumes about of 10GB of RAM.
Assuming some people don't use Model.all in their code,
it's important to disable that feature on demand

@nattfodd nattfodd force-pushed the disable_all_index_on_demand branch from 28b6238 to a2aa543 Compare June 12, 2017 14:12
Allows to disable creating of my:core:key:all kind of keys if they are not needed.
Getting rid of these keys is important feature, because those keys consume a huge amount of memory
(one single *:all key for 158m records consumes for about of 10GB of ram)
Assuming some people don't use Model.all in their code,
it's important to disable that feature on demand
@nattfodd nattfodd force-pushed the disable_all_index_on_demand branch from a2aa543 to 5ef9bea Compare June 12, 2017 14:15
@Tonkpils
Copy link
Collaborator

Considering this is out in the wild, I'm a bit skeptic to introduce this as an opt-in feature as developers that update the gem would have to add the code to maintain current functionality.

What do you think about making this an opt-out option instead so that we can safely release a new 2.x version instead and make a future release make this opt-in. That would give you the ability to opt-out as needed for now without breaking others codebase.

Also, any thoughts on the behavior of all when this is turned off? Should it log a warning or raise an exception rather than returning an empty array when this is turned off?

… set to false

By default, redis returns emtpy array when the desired key does not exist.
This behavior may mislead a developer, since he just disabled that key to be used.
So instead throw error to bring an attention to it
@nattfodd
Copy link
Contributor Author

nattfodd commented Jun 13, 2017

Considering this is out in the wild, I'm a bit skeptic to introduce this as an opt-in feature as developers that update the gem would have to add the code to maintain current functionality.

They don't have to. "all" index is still enabled by default, so default behavior did not change. So if no use_all_index option is present, everything will remain the same as before. So no breaking code stuff.

def all_index_enabled?
  @use_all_index == true || @use_all_index.nil?
end

Or, are you talking more about semantic stuff like disable_all_index true is more clear than enable_all_index false?

Also, any thoughts on the behavior of all when this is turned off? Should it log a warning or raise an exception rather than returning an empty array when this is turned off?

I thought on this, but wasn't sure about the details. You're right. I've added another commit introducing error when user is trying to get :all records with that option disabled.

@Tonkpils
Copy link
Collaborator

Sorry to get back at this so late, I've been pretty swamped with life 😄

Or, are you talking more about semantic stuff like disable_all_index true is more clear than enable_all_index false?

Yeah, I was talking about the name as it originally confused me. Though reading the code it makes sense the way you implemented it. If you don't mind making the name change that would be great, I'll merge this and release a new minor version update 👍

@Tonkpils
Copy link
Collaborator

Actually, disregard...thinking about it, it makes sense to leave it as is if in the future we make this an opt-in feature the name will make better sense

@Tonkpils Tonkpils merged commit c32c716 into rpush:master Jun 23, 2017
@nattfodd
Copy link
Contributor Author

Thank you! Are you going to release a minor version with this update soon? (I just want to use this fix in our production, so we prefer setting version instead of github's master branch)

@Tonkpils
Copy link
Collaborator

Thank you! Are you going to release a minor version with this update soon? (I just want to use this fix in our production, so we prefer setting version instead of github's master branch)

Yeah, I'll do that today as I'm at GORUCO for the day and I tend to do a lot of OSS work when I'm at conferences 😄

@Tonkpils
Copy link
Collaborator

@nattfodd I've released version 2.1.0 of the gem which includes your change! Thank you for this! 🎉

@nattfodd nattfodd deleted the disable_all_index_on_demand branch February 26, 2021 14:23
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.

None yet

2 participants