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

Expose Marshal as a legit SchemaCache serialization strategy #38432

Merged

Conversation

@kytrinyx
Copy link
Contributor

kytrinyx commented Feb 11, 2020

Summary

This lets people choose between Marshal and YAML as a serialization strategy, but defining the schema dump path filename for each database connection, either with the suffix .dump (for Marshal) or .yml (for YAML).

The default behavior is still YAML.

Other Information

The Marshal behavior was replaced with YAML in 2016, and then in 2019 an attempt was made to remove it. It was put back the next day. In #38347 I suggested deprecating it, since it seemed like the intention was to be able to remove it, though I had not found any issues/PRs discussing it.

@tenderlove suggested that instead of removing it, we should make it configurable.

Here I've pushed the load and dump behavior into the schema class itself, in order to group the serialization logic somewhat.

@rails-bot rails-bot bot added the activerecord label Feb 11, 2020
@kytrinyx kytrinyx force-pushed the kytrinyx:schema-cache-serialization-strategy-2 branch 2 times, most recently from a9895d7 to b2bcf76 Feb 11, 2020
@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 12, 2020

I have updated the PR, but the builds seem to still be failing. It's not clear to me whether or not the failures are related to my change.

@tenderlove tenderlove self-assigned this Feb 13, 2020
kytrinyx added 2 commits Feb 7, 2020
@kytrinyx kytrinyx force-pushed the kytrinyx:schema-cache-serialization-strategy-2 branch from b2bcf76 to 9a356fc Feb 13, 2020
@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 13, 2020

I paired with @seejohnrun to figure out the failing builds. It turns out that moving the ActiveRecord::Migrator.current_version method call was creating an empty sqlite database that wasn't getting cleaned up and was mucking up the railtie tests.

I think we've sorted it out (waiting for the builds to finish).

@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 13, 2020

@tenderlove you were right about the speed issue (obviously, in hindsight :-))

This runs 500 iterations through dumping and loading all of our databases on GitHub dotcom locally on my computer.

                          user        system    total      real
YAML.dump                 565.644474  26.591381 592.235855 (823.312556)
Marshal.dump              380.641957  24.830863 405.472820 (632.367425)
YAML.load                 125.253823  0.745503  125.999326 (126.236146)
Marshal.load              13.110148   0.383886   13.494034 ( 13.545109)
@rafaelfranca rafaelfranca merged commit 40c7c5e into rails:master Feb 13, 2020
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #66989 passed (20 minutes, 52 seconds)
Details
@kytrinyx kytrinyx deleted the kytrinyx:schema-cache-serialization-strategy-2 branch Feb 13, 2020
@saiqulhaq

This comment has been minimized.

Copy link
Contributor

saiqulhaq commented Feb 21, 2020

what should I do to use this Marshal as SchemaCache? which config file that should be updated?

@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 21, 2020

@saiqulhaq You need to update your database.yml file, and give the config a value called schema_cache_path: path/to/your/file.dump. If the extension name is .dump then it will choose Marshal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.