Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

feat(chore): add default versioning & hostname on cache key #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marc-arnoult
Copy link

Description

Added hostname and a new configuration to set a version or a default one generated at build time.

Related Issue

Motivation and Context

Currently, we are missing some functionality to be able to use the SSR cache correctly with our implementation of VSF (Magento2 integration).

Actually when a new version is deployed, we can get the old version from the cache and it can leverage multiple bugs on a deployment.

The other point is that we are using subdomain for our multistore, but the cache key is only used with the route value for example / for the homepage. With that a page for a subdomain will be cache for every subdomain of the application.

How Has This Been Tested?

Checked subdomain are cached separately.
Checked the keys format in redis.
Checked if a new key is created after a new build.

Screenshots (if appropriate):

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.

@bloodf bloodf self-assigned this Feb 21, 2022
@jonathanribas
Copy link

Hi @bloodf, is it possible to validate this MR please?

@filipsobol
Copy link
Contributor

filipsobol commented Mar 3, 2022

This is a great contribution, but I have some issues with the proposed approach. Besides the breaking change caused by moving the Redis client configuration to the redisConfig property, versioning cache can cause the following issues:

  1. If options.version is not provided, the integration will generate a random string. If only one instance of the app instance is running, this is not an issue. However, if multiple instances are running, they will not share the cache, but instead, each will generate separate entries for the same pages.
  2. After every deployment, the version will change, and integration will start creating new entries without deleting the old ones. After a few deployments, it can become an issue, especially because Redis is an in-memory store.

I think that it's much easier to add cache invalidation as part of the deployment process. As described in the documentation, you can invalidate the whole cache by making a request like this:

http://localhost:3000/cache-invalidate?key=<INVALIDATION KEY>&tags=*

Let me know what you think about this because I might be wrong.

@marc-arnoult
Copy link
Author

This is a great contribution, but I have some issues with the proposed approach. Besides the breaking change caused by moving the Redis client configuration to the redisConfig property, versioning cache can cause the following issues:

  1. If options.version is not provided, the integration will generate a random string. If only one instance of the app instance is running, this is not an issue. However, if multiple instances are running, they will not share the cache, but instead, each will generate separate entries for the same pages.

Yes i see, i will remove it. By default it will be empty, we should warn the users in the documentation for this part. What do you think ?

  1. After every deployment, the version will change, and integration will start creating new entries without deleting the old ones. After a few deployments, it can become an issue, especially because Redis is an in-memory store.
    I think that it's much easier to add cache invalidation as part of the deployment process. As described in the documentation, you can invalidate the whole cache by making a request like this:
http://localhost:3000/cache-invalidate?key=<INVALIDATION KEY>&tags=*

Let me know what you think about this because I might be wrong.

I think for this part we will have to let the developers do what they want to do. What advertises a problem when I was working on VSF1 with many items being deleted at once caused a problem with Redis when deploying.
By default we have a TTL, so items will be purged but not all at once.
And if they want to clear it all at once a script should be good enough ?

@filipsobol
Copy link
Contributor

After thinking about this more, I changed my mind and agree with your approach.

  1. It's better to generate a new random version for every instance than to risk serving outdated cache.
  2. Cache can be shared between multiple instances if options.version is provided.
  3. As you said, removing the old cache is solved by defaultTimeout (TTL), which I forgot is enabled by default.

I will submit a code suggestion to improve DX and avoid breaking changes.

I'd like you to update this document to include information about the new property (options.version), how to use it, and why (multi-instance setup).

Copy link
Contributor

@filipsobol filipsobol left a comment

Choose a reason for hiding this comment

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

Please test these changes before merging

src/index.js Outdated

export default function RedisCache (options) {
const client = new Redis(options.redisConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const client = new Redis(options.redisConfig);
const { version, ...redisConfig } = options;
const client = new Redis(redisConfig);
const fallbackVersion = crypto.randomBytes(15).toString('hex');
if (!version) {
Logger.warn('The `version` property is missing in the `@vue-storefront/redis-cache` package configuration. In a multi-instance setup, this will result in a separate cache for every instance. Please refer to Redis driver documentation for more details.');
}

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/index.js Outdated
Comment on lines 4 to 5
const defaultVersion = crypto.randomBytes(15).toString('hex')

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const defaultVersion = crypto.randomBytes(15).toString('hex')

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/index.js Show resolved Hide resolved
src/index.js Outdated
Comment on lines 11 to 15
const version = options.version || defaultVersion
const hostname = context.req.hostname
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const version = options.version || defaultVersion
const hostname = context.req.hostname
const version = options.version || fallbackVersion;
const hostname = context.req.hostname;

Copy link
Author

Choose a reason for hiding this comment

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

Done

marc-arnoult added a commit to marc-arnoult/vue-storefront that referenced this pull request Mar 8, 2022
@marc-arnoult
Copy link
Author

@filipsobol for the documentation vuestorefront/vue-storefront#6666

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants