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.

Sign up for GitHub

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

Addressing #838, STI issues when code is reloaded in development, fix… #1064

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Addressing #838, STI issues when code is reloaded in development, fix… #1064

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81536dd
Addressing #838, STI issues when code is reloaded in development, fix…
vanboom Nov 10, 2023
11df40b
refactoring index to model map concern into Registry to address #838 …
vanboom Nov 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions elasticsearch-model/lib/elasticsearch/model/adapters/multiple.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,16 +106,16 @@ def __ids_by_type
# @api private
#
def __type_for_hit(hit)
@@__types ||= {}

key = "#{hit[:_index]}::#{hit[:_type]}" if hit[:_type] && hit[:_type] != '_doc'
key = hit[:_index] unless key

@@__types[key] ||= begin
Copy link
Author

@vanboom vanboom Nov 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I put Benchmark.measure around this code and contrary to my expectations, the original hash method was WAY. We will have to re-think this change, maybe the method of storing the class->index_name mapping can be improved to mitigate #838 with performance.
image

NOTE: my models use a Proc for index_name in a multi-tenant application so we are attempting to use an apartment qualified index name. Seems the Proc based index_name is causing the performance hit.

Registry.all.detect do |model|
(model.index_name == hit[:_index] && __no_type?(hit)) ||
(model.index_name == hit[:_index] && model.document_type == hit[:_type])
end
# DVB -- not sure the ramifications of this - but do not memoize the model/klass
# I do not think that caching the types is necessary, minimal processing savings
# at the expense of broken STI autoloading in development, see #848
Registry.all.detect do |model|
(model.index_name == hit[:_index] && __no_type?(hit)) ||
(model.index_name == hit[:_index] && model.document_type == hit[:_type])
end
end

Expand Down
7 changes: 6 additions & 1 deletion elasticsearch-model/lib/elasticsearch/model/multimodel.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,12 @@ def self.all
# Adds a model to the registry
#
def add(klass)
@models << klass
# Detect already loaded models and ensure that a duplicate is not stored
if i = @models.index{ |_class| _class.name == klass.name }
@models[i] = klass
else
@models << klass
end
end

# Returns a copy of the registered models
Expand Down