-
Notifications
You must be signed in to change notification settings - Fork 0
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
[PLATI-1363] Use Identity as the default compressor & serializer #5
base: braze-main
Are you sure you want to change the base?
Conversation
lib/dalli/server.rb
Outdated
@@ -26,12 +26,12 @@ class Server | |||
:value_max_bytes => 1024 * 1024, | |||
# surpassing value_max_bytes either warns (false) or throws (true) | |||
:error_when_over_max_size => false, | |||
:compressor => Compressor, | |||
:compressor => (defined?($TESTING) && $TESTING) ? Compressor : Dalli::Identity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dalli's pattern seems to be omitting the shared module prefix.
:compressor => (defined?($TESTING) && $TESTING) ? Compressor : Dalli::Identity, | |
:compressor => (defined?($TESTING) && $TESTING) ? Compressor : Identity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should arguably update the tests to pass Compressor
instead of use the default, if they fail otherwise. I don't feel particularly strongly about that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that initially but it required a good amount of changes. I think we should probably keep the number of changes to a minimum to avoid diverging from upstream too much. Also I think doing it this way will allow us to bring in changes from upstream more easily in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in Rails 7.1 prevent us from passing these in from
platform
so this change adds theIdentity
serializer/compressor and makes it the default.
Can you explain more about this?
I would rather stop the bleeding (more and more changes to our fork diverging from upstream). I have to imagine that the newer version of dalli
works with newer Rails and does not have an Identify compressor nor serializer.
@jasonpenny We currently use Dalli, by default, also uses its own serializer & compressor. I think to avoid serializing & compressing entries twice we've been passing in But as of Rails 7.1, we can only specify either a cc @naveg |
Changes in Rails 7.1 prevent us from passing these in from
platform
so this change adds theIdentity
serializer/compressor and makes it the default.