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

Enable DB table names customization #535

Merged
merged 1 commit into from
Mar 3, 2022
Merged

Enable DB table names customization #535

merged 1 commit into from
Mar 3, 2022

Conversation

dimvic
Copy link
Contributor

@dimvic dimvic commented Mar 2, 2022

With this patch it is possible to assign custom table names for GoodJob::Process and GoodJob::ActiveJobJob and have good_job function as usual.

It enables the use of a common database for two or more schedulers while keeping the dashboard functional.

My motive was working on two apps that share a database and both need to run a scheduler and we want to run good_job because well, it's too good for our use case :). I believe this is a legit, while ugly, requirement in some projects.

There is no need to complicate things by making the migrations take the custom table names into account for upgrades. It is possible, but the added complexity would possibly make migrations fragile and certainly harder to reason about. After all, anyone who opts to change the table name this way would review migrations coming from updates before running them. Even if not, they would fail and the person would iterate.

The table names have been added without quoting into the SQL code because, well, it's a table name.

I like to believe that the only reason to consider not including this is that future code would need to use table names via model classes instead of hardcoded, which is not necessarily a bad thing.

Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

This is fantastic! And simple 🙌

@bensheldon
Copy link
Owner

@dimvic fyi, in your application, you should be overriding the table name for GoodJob::Execution too. GoodJob's scheduler works off of executions (which is an individual performance of a job). The awkwardly named (till the next major release of GoodJob) ActiveJobJob is used for representing a more ergonomic model in the Dashboard.

This PR is great btw, just wanted to clarify the models.

@dimvic
Copy link
Contributor Author

dimvic commented Mar 2, 2022

Thanks!

Would you like me to add tests for this? Would only take a couple more introduced in spec/engine. I'd sleep better too.

Just for reference, being explicit, to use different table names you need to 1. adjust the generated migrations and 2. add this to app/environment config or an initialiser:

GoodJob::Process.table_name = 'my_good_job_processes'
GoodJob::ActiveJobJob.table_name = 'my_good_jobs'
GoodJob::Execution.table_name = 'my_good_jobs'

@bensheldon
Copy link
Owner

@dimvic yep, that's exactly right. I don't think this needs tests because it's largely how ActiveRecord works.

It does make me think that I should make ActiveJobJob delegate its tablename to Execution so that you only have to set that on the Execution itself. Let me merge/release this change to unblock you, and then I'll make another PR for your feedback.

@bensheldon bensheldon merged commit ca92be4 into bensheldon:main Mar 3, 2022
@bensheldon bensheldon added the refactor Code changes that do not introduce new features label Mar 3, 2022
@bensheldon
Copy link
Owner

@dimvic I released v2.11.2 with this change 🎉

@dimvic
Copy link
Contributor Author

dimvic commented Mar 3, 2022

Thank you very very much. I agree that since the table name is shared between the two it is would be beneficial to have it delegated, but I don't see much more use to it other than making it easier to customise table names. Ping me if you'd like me to do something more about this, I'd be more than happy to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code changes that do not introduce new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants