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

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 7 additions & 9 deletions elasticsearch-model/lib/elasticsearch/model/adapters/multiple.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,15 @@ 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

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
key = if hit[:_type] && hit[:_type] != '_doc'
"#{hit[:_index]}::#{hit[:_type]}"
else
hit[:_index]
end

# DVB -- #838, refactor the lookup of index name to model into the Registry
Registry.lookup(key, hit[:_type])
end

def __no_type?(hit)
Expand Down
52 changes: 50 additions & 2 deletions elasticsearch-model/lib/elasticsearch/model/multimodel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ module Elasticsearch
module Model

# Keeps a global registry of classes that include `Elasticsearch::Model`
#
# Keeps a global registry of index to class mappings
class Registry
def initialize
@models = []
@indexes = {}
end

# Returns the unique instance of the registry (Singleton)
Expand All @@ -45,17 +46,64 @@ def self.all
__instance.models
end

def self.indexes
__instance.indexes
end

def self.add_index(index, model)
__instance.add_index(index, model)
end

# 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
# clear the cached index map (autoloading in development causes this)
@indexes.clear
else
@models << klass
end
end

def add_index(index, model)
@indexes[index] = model
end

# Returns a copy of the registered models
#
def models
@models.dup
end

def indexes
@indexes.dup
end

##
# Find the model matching the given index and document type from a search hit
# Cache the index->model mapping for performance
# Clear the index cache when models are reloaded
def self.lookup(index, type=nil)
if Registry.indexes.has_key?(index)
# Cache hit
Registry.indexes[index]
else
# Cache bust
model = if type.nil? or type == "_doc"
# lookup strictly by index for generic document types
Registry.all.detect{|m| m.index_name == index}
else
# lookup using index and type
Registry.all.detect{|m| m.index_name == index and model.document_type == type}
end
# cache the index to model mapping
Registry.add_index(index, model)
model
end
end

end

# Wraps a collection of models when querying multiple indices
Expand Down