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

Remove memoization from index name #318

Conversation

mikeyhogarth
Copy link
Contributor

Simple change but maybe one that needs some discussion. The reason for making this change is that the value that needs to be assigned to @index_name may change between calls to index_name, such as between requests in multitenant environment.

If for example the default_prefix were set to a lambda or were overridden to be something dynamic, it wouldn't be re-initialised between requests.

We can sort this out by just not memoizing that variable, however this does introduce a performance overhead (it'll re-evaluate the regex/demodulize etc. every time and will create a new variable) however I think this overhead is very minor compared to the benefit.

If you'd prefer not to have the overhead at all I could introduce this as a configurable option (dynamic_index_names: true or something) but then that's maybe a step too far.

@mikeyhogarth
Copy link
Contributor Author

Hmm this appears to cause other tests in the test suite to fail :(

@pyromaniac do you know if the memoization here (not being able to change the non-suggested index name after it's been set once) is desired functionality? The fact that I've broke the test suite by removing memoization would suggest so

@pyromaniac
Copy link
Contributor

Well, if we are going to remove memoization, we have to get rid of instance variables completely. Also we have to extract name.sub(/Index\Z/, '').demodulize.underscore and similar statements to memoize them separately.

@pyromaniac
Copy link
Contributor

But I'm ok with memoization removement

@pyromaniac
Copy link
Contributor

Hey man! Are you going to update this? I would like to merge this PR

@pyromaniac
Copy link
Contributor

In master already

@pyromaniac pyromaniac closed this Jul 3, 2017
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