Poc multi adapter single model single process#49
Poc multi adapter single model single process#49hmcguire-shopify wants to merge 11 commits intomainfrom
Conversation
|
|
||
| # Set the table name and clear cached state that depends on it | ||
| def table_name=(value) | ||
| @table_name = nil |
There was a problem hiding this comment.
I think this should be value right
There was a problem hiding this comment.
yes it should (or it should be removed because it isn't used)
| def model_schema # :nodoc: | ||
| context_key = current_schema_context_key | ||
| @model_schemas ||= {} | ||
| @model_schemas[context_key] ||= Schema.new(self, context_key) |
There was a problem hiding this comment.
So the model schema is loaded lazily only when the current context needs it. Makes sense. But don't we need to iterate through all the schemas when we load the model so we can make sure we generate the right attribute methods? Or are we expecting DBs to always have the same columns and thus the same methods (is that even enough to generate the needed proper methods 🤔 )
| # Multiple pools (read replicas, shards) can share the same key, meaning | ||
| # they share cached schema information. Defaults to "default". | ||
| def schema_context_key # :nodoc: | ||
| configuration_hash.fetch(:schema_context, "default").to_s |
There was a problem hiding this comment.
Is it worth asking do we really need this config? The vision is we are switching between different DB adapters right? So can we just switch based on that? Is there really a use case to have arbitrary schema contexts?
hmcguire-shopify
force-pushed
the
poc-multi-adapter-single-model-single-process
branch
2 times, most recently
from
3ceeeb8 to
690e14a
Compare
hmcguire-shopify
force-pushed
the
poc-multi-adapter-single-model-single-process
branch
from
4950c19 to
02a744d
Compare
| def symbol_column_to_string(name_symbol) | ||
| @symbol_column_to_string_name_hash ||= column_names.index_by(&:to_sym) | ||
| @symbol_column_to_string_name_hash[name_symbol] | ||
| end |
There was a problem hiding this comment.
This one's also interesting: maybe it should just be a global hash so that models with the same column names could share?
There was a problem hiding this comment.
Nope, its a proxy for column_names/column_hash: https://github.com/rails/rails/pull/34197
| end | ||
|
|
||
| # Returns the column object for a named attribute | ||
| def column_for_attribute(name) |
There was a problem hiding this comment.
Maybe this can be global too... the columns_hash will be per context anyways and nothing here is cached?
The issue is Active Model defines `attribute_types` and caches it. `ModelSchema` brought the value inside Active Record because it depends on `default_attributes` (which we want to be per-context). However, the implementation was missing the addition of an override for Active Model's `attribute_types` implementation.
since it depends on columns_hash, which is ModelSchema aware
The last few commits were trying to fix issues with methods defined in Active Model that now need to be multi-context aware. However, moving the method from Attributes to ModelSchema actually _undefined_ it because ModelSchema was included before Attributes, so the one in AttributeRegistration was being defined last. This commit fixes the issue by moving ModelSchema after Attributes so that our context aware methods win.
Other methods (type_for_column) depend on the current order, so instead I'll move the model_schema aware methods to better places
table_name= was previously clearing some instance variables, but those variables are now inside ModelSchema so they need to be cleared there. attribute_names ha a similar issue. Its clearing was moved inside ModelSchema, but should not be, since one of our constraints is that attribute_names should be consistent across contexts.
arel_table and predicate_builder are now inside the Schema, so they don't need to be cleared here. Instead, they get cleared by reseting the model_schemas hash, which is done in a child inherited definition.
At the moment it fails because find_by_statement_cache is being shared between contexts, which I know we need to fix.
Previously it was global across contexts which would cause issues if contexts don't generate the same queries.
For some reason I can't get our connected? to return true. It's unclear whether its a (monkey patching) bug in our app, but it shouldn't be necessary here anyways since we're only looking at statically defined configuration. This rescue may not be necessary, there's one test in Rails' suite that fails without it but it seems suspect.
hmcguire-shopify
force-pushed
the
poc-multi-adapter-single-model-single-process
branch
from
02a744d to
0936bd9
Compare
3 participants
Opening here so I can write notes/solicit feedback
TODOS:
send