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 Rails 7.1 Marshalling format #28609

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

byroot
Copy link
Contributor

@byroot byroot commented Jan 5, 2024

This replace the workaround added in #24142 and also fixes #27622 so #28521 can be reverted.

That new format isn't backward compatible, so normally you are supposed to deploy Rails 7.1 without that new format enabled first, but this can be sidestepped by invalidating old cache entries, which this PR does by changing the cache prefix.

cc @ClearlyClaire @Gargron

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (43d800a) 84.78% compared to head (f1394ed) 84.75%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #28609      +/-   ##
==========================================
- Coverage   84.78%   84.75%   -0.03%     
==========================================
  Files        1039     1039              
  Lines       28259    28175      -84     
  Branches     4560     4548      -12     
==========================================
- Hits        23960    23881      -79     
+ Misses       3139     3136       -3     
+ Partials     1160     1158       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This replace the workaround added in mastodon#24142
and also fixes mastodon#27622 so
mastodon#28521 can be reverted.

That new format isn't backward compatible, so normally you are
supposed to deploy Rails 7.1 without that new format enabled
first, but this can be sidestepped by invalidating old cache
entries, which this PR does by changing the cache prefix.
ClearlyClaire
ClearlyClaire previously approved these changes Jan 5, 2024
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Looks fine to me. One downside is that it'll leave old keys in the cache, but most should only persist for 10 minutes, some persist for up to 1 day, and a handful for up to a week.

I also wonder if we shouldn't take the opportunity to use MessagePack?

app/controllers/concerns/cache_concern.rb Outdated Show resolved Hide resolved
ClearlyClaire
ClearlyClaire previously approved these changes Jan 5, 2024
@byroot
Copy link
Contributor Author

byroot commented Jan 5, 2024

One downside is that it'll leave old keys in the cache, but most should only persist for 10 minutes, some persist for up to 1 day, and a handful for up to a week.

I guess it depends how the Redis is configured. By default the maxmemory-policy is volatile-lru so these unreachable keys will simply be evicted.

ClearlyClaire
ClearlyClaire previously approved these changes Jan 5, 2024
casperisfine pushed a commit to casperisfine/rails-settings-cached that referenced this pull request Jan 5, 2024
Referencing `ActiveRecord::Base` as soon as the gem is required
cause Active Record being loaded earlier than it should and
some Rails configurations not to take effect.

Ref: rails/rails#49827
Ref: mastodon/mastodon#28609
casperisfine pushed a commit to casperisfine/rails-settings-cached that referenced this pull request Jan 5, 2024
Referencing `ActiveRecord::Base` as soon as the gem is required
cause Active Record being loaded earlier than it should and
some Rails configurations not to take effect.

Ref: rails/rails#49827
Ref: mastodon/mastodon#28609
@byroot
Copy link
Contributor Author

byroot commented Jan 5, 2024

As mentioned, I added the workaround back, so this can ship. I'll see about cleaning up the settings gem if I find the time.

ClearlyClaire
ClearlyClaire previously approved these changes Jan 5, 2024
@ClearlyClaire
Copy link
Contributor

As mentioned, I added the workaround back, so this can ship. I'll see about cleaning up the settings gem if I find the time.

Looks good to me. I think I prefer to have the config set directly in config/application.rb to make sure there is no cache format change at runtime, but I can do that in a separate PR.

Likewise, I think it would be a good thing to find and fix the places where ActiveRecord is initialized early, but that can be done separately, and you've given us plenty of information about how to find those cases! Thanks again

@byroot
Copy link
Contributor Author

byroot commented Jan 5, 2024

I think I prefer to have the config set directly in config/application.rb to make sure there is no cache format change at runtime

I've amended my commit to do that.

Copy link
Sponsor Member

@renchap renchap left a comment

Choose a reason for hiding this comment

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

Looks good to me. Yay for removing ugly hacks.

This will cause empty caches in many places when deploying this commit, but I think we did it before and did not see really bad performance following it.

I also wonder if we shouldn't take the opportunity to use MessagePack?

This is probably a good idea, probably using the paquito gem? But this should probably be in another PR, as I expect we will have some additional work to do to ensure all our shapes are serialisable.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Jan 5, 2024
Merged via the queue into mastodon:main with commit 5a6d533 Jan 5, 2024
28 checks passed
vmstan pushed a commit to vmstan/mastodon that referenced this pull request Jan 5, 2024
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.

NoMethodError (undefined method `title' for #<CustomFilter...
4 participants