Skip to content

Prep to remove Readthis in favor of ActiveSupport::Cache::RedisCacheStore#5337

Merged
zachmargolis merged 14 commits intomainfrom
margolis-remove-readthis
Aug 31, 2021
Merged

Prep to remove Readthis in favor of ActiveSupport::Cache::RedisCacheStore#5337
zachmargolis merged 14 commits intomainfrom
margolis-remove-readthis

Conversation

@zachmargolis
Copy link
Contributor

  • Also just use plain redis connections elsewhere in the app

Builds on #5335 (because that renamed REDIS_POOL to READTHIS_POOL) --- happy to flip the order on these if anybody feels strongly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🚨 ok so this is going to be a doozy ---- the Readthis gem serializes and deserializes via Marshal (😭) and also does some compression, and we use this for important data that is not strictly caching (ServiceProviderRequests and proofing intermediate results), so we need to handle this carefully

I think we have a few options:

  1. Leave in this gem forever because data migration is scary
  2. Write our own code that reads/writes the Marshal + Deflate compression to migrate
  3. Dual reads/write: leave in gem while we read from old locations, but then always write to newer locations (ex with JSON serialization)

I think 3 is the safest and helps us minimize our dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going with (3), but also going to namespace the new writes to make sure we don't write to JSON parse Marshal'd data or otherwise

Copy link
Contributor

@mitchellhenke mitchellhenke Aug 30, 2021

Choose a reason for hiding this comment

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

3 makes sense and is the lowest risk 👍🏼

@zachmargolis zachmargolis marked this pull request as draft August 25, 2021 22:50
@zachmargolis zachmargolis force-pushed the margolis-rate-limit-risc branch from 27fa159 to a8e7dcc Compare August 26, 2021 20:34
Base automatically changed from margolis-rate-limit-risc to main August 26, 2021 21:05
@zachmargolis zachmargolis force-pushed the margolis-remove-readthis branch from 7d04164 to 0fe4737 Compare August 26, 2021 21:09
@zachmargolis zachmargolis changed the title Remove Readthis in favor of ActiveSupport::Cache::RedisCacheStore Prep to remove Readthis in favor of ActiveSupport::Cache::RedisCacheStore Aug 27, 2021
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the namespace should separate new data from old data, it means there will essentially be a reset on the rack-attack values after a deploy, but I think the timeouts are short-lived enough things will be okay?

@zachmargolis zachmargolis changed the base branch from main to margolis-redis-namespace August 27, 2021 23:10
@zachmargolis zachmargolis marked this pull request as ready for review August 27, 2021 23:43
Base automatically changed from margolis-redis-namespace to main August 30, 2021 14:53
@zachmargolis zachmargolis force-pushed the margolis-remove-readthis branch from 469c165 to eedf20a Compare August 30, 2021 16:07
@zachmargolis
Copy link
Contributor Author

This is ready to review!

@zachmargolis zachmargolis merged commit eb5e617 into main Aug 31, 2021
@zachmargolis zachmargolis deleted the margolis-remove-readthis branch August 31, 2021 16:34
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.

2 participants