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

Performance improvements and various fixes #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gencer
Copy link

@gencer gencer commented Nov 20, 2019

I've tried to be more descriptive as possible. Please see the commit description for why and how this fixes applied.

This PR includes:

  • Limiting ObjectSpace to Sequel::Model::ClassMethods only.
  • Using drop on commit while creating temporary tables. This will saves redundant SQL query from sending to the server.
  • Adding an exception handling and returning whole row ins case of temporary table is not found in somehow.
  • Removing variable cache due to fast select mechanism. This will allow all tables and models from being mapped to the database.

Please review and comment for the changes if you think there might be a different changes made.

This fixes #3.

With this small change, we will almost instantly filter classes and tear down to the Sequel level. This allow us to only 
 `select()` Sequel models instead of the whole Rails workspace. Thus, we do not need to cache the variable. Because, Rails uses eager load or may use threaded model. This means, when this method fired, only few models would be loaded at that time. This will lead missing models for tables.

Note that we also used `c.implicit_table_name`. This is necesary for who uses (like us) multiple Sequel Databases at the same time on single Rails App. `DBModel::TableModel.klazz` will require `implicit_table_name` to demodulize, undersrcore and proper pluralization.
Sequel can send `drop` on commit by default. This can be activated via `on_commit: :drop`. This will minimize queries sent to the Server.
We do not need `self.` calls here. They are redundant.
We also added an exception handling here. Problem here is that, when trigger executed and temporary table does not exists (for example we forgot to send :bulk_audit commands) then there is a issue raises. In this case, if anything goes wrong and transaction is not found, whole object will return immediately without breaking anything.
It seems your tests didn't liked this...
@gencer
Copy link
Author

gencer commented Nov 20, 2019

👌 Tests have passed!

@@ -12,22 +12,21 @@ def self.apply(model, opts={})

module SharedMethods
def model_to_table_map
@@model_to_table_map ||= ObjectSpace.each_object(Class).select do |klazz|
ObjectSpace.each_object(Sequel::Model::ClassMethods).select do |klazz|
Copy link
Contributor

Choose a reason for hiding this comment

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

Sequel::Plugins::BulkAudit::ClassMethods can be even better

Copy link
Author

Choose a reason for hiding this comment

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

In this case do we need to check if Model has BulkAudit in next line?

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.

Issues & Performance Improvements
2 participants