Skip to content

IPLAYER-45548: Replace cache key package version with schema version#42

Merged
cjewell47 merged 6 commits intomainfrom
45548-Replace-HTTP-Transport-Cache-Version-with-schema-version
Oct 15, 2024
Merged

IPLAYER-45548: Replace cache key package version with schema version#42
cjewell47 merged 6 commits intomainfrom
45548-Replace-HTTP-Transport-Cache-Version-with-schema-version

Conversation

@cjewell47
Copy link
Contributor

@cjewell47 cjewell47 commented Oct 14, 2024

Description

Right now HTTP-Transport-Cache includes the package version number in its key. This means for changes where there is no change to the data stored - e.g. readme changes, we are diluting the cache for where that version is updated. To fix this we have replaced the package version with a config value that we can update when we actually make a data change

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@cjewell47 cjewell47 marked this pull request as ready for review October 14, 2024 12:47
@cjewell47 cjewell47 requested a review from a team as a code owner October 14, 2024 12:47
Copy link

@drrobharper drrobharper left a comment

Choose a reason for hiding this comment

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

I think I disagree with replacing the user-agent in the key with anything. We are caching responses, which are completely unrelated to the code used to make the requests or cache them.

Copy link
Contributor

@chr15stevens chr15stevens left a comment

Choose a reason for hiding this comment

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

I'd argue against this change tbh. I think in some ways that you aren't addressing the core issue of people not bumping consumers when they make an update.

The same thing will happen with this change. They'll make a change, increment the new config version and then not update it everywhere.

@drrobharper
Copy link

I'd argue against this change tbh. I think in some ways that you aren't addressing the core issue of people not bumping consumers when they make an update.

The same thing will happen with this change. They'll make a change, increment the new config version and then not update it everywhere.

We can never bump a dependency everywhere all at once, so if the user-agent/rest-client version is left in the cache key the cache will always become fragmented for a period of time. I just don't see what purpose it serves. The only reason you would want to add something into the cache key is for a breaking change to how objects are stored in the cache. IMHO if there was a requirement to do that we could deal with that at the time

Copy link

@drrobharper drrobharper left a comment

Choose a reason for hiding this comment

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

Potential addition to the README but otherwise LGTM

cjewell47 and others added 2 commits October 15, 2024 14:34
Co-authored-by: Rob Harper <misforturob@gmail.com>
@cjewell47 cjewell47 merged commit 3ec280d into main Oct 15, 2024
@cjewell47 cjewell47 deleted the 45548-Replace-HTTP-Transport-Cache-Version-with-schema-version branch October 15, 2024 14:00
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.

4 participants