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

Refactor/remove memoization of index name prefix #382

Conversation

JanBussieck
Copy link

@JanBussieck JanBussieck commented May 2, 2016

There is an open pull request #314 concerning the issue that for multitenant applications the index prefix needs to be set dynamically (#225) which is currently not possible since the index name is memoized. I think there has been some misunderstanding concerning the point of the memoization which is not performance related, but rather a result of the fact that index_name is both a getter and setter (unfortunately) and it should be memoized if set explicitly.

This behavior is preserved in the changes I propose by saving the index_name_stem when it is set while allowing the prefix to be reevaluated every time index_name is accessed.

This call relies on @index_name already being memoized, and would otherwise cause infinite recursion
@pyromaniac
Copy link
Contributor

This code looks really cryptic, the control flow is spread all over the source. I would prefer to have less side-effects in methods.

@JanBussieck
Copy link
Author

You are right of course, I moved the memoization into the private method index_name_stem. We'll have the awkward behavior of either setting or computing a value in a single method either way, I moved this down to index_name_stem which is the actual value we want to memoize.

@pyromaniac
Copy link
Contributor

Alright, it looks much better. Could I ask you to add specs to consolidate this behavior? Also I'm going to think about this a little bit more over the weekend.

@JanBussieck
Copy link
Author

Sure will do. If you have any other suggestions concerning this change, I'd be happy to incorporate them.

@JanBussieck
Copy link
Author

Any idea why this little change causes so much breakage in other places?

@pyromaniac
Copy link
Contributor

Because everything else is counting on the name cache, I'm still going to think how to make it the best way, unfortunately I didn't have a chance yet :(

@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