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

feat: new read model for client feature toggle cache #8975

Merged
merged 3 commits into from
Dec 13, 2024
Merged

feat: new read model for client feature toggle cache #8975

merged 3 commits into from
Dec 13, 2024

Conversation

sjaanus
Copy link
Contributor

@sjaanus sjaanus commented Dec 13, 2024

This is based on the exising client feature toggle store, but some alterations.

  1. We support all of the querying it did before.
  2. Added support to filter by featureNames
  3. Simplified logic, so we do not have admin API logic
  • no return of tags
  • no return of last seen
  • no return of favorites
  • no playground logic

Next PR will try to include the revision ID.

Copy link

vercel bot commented Dec 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2024 8:08am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2024 8:08am

Copy link
Contributor

@sjaanus, core features have been modified in this pull request. Please review carefully!

Copy link
Contributor

github-actions bot commented Dec 13, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

Personally, I don't like the fact of having a specific read model for the cache. The cache should cashe a read model for something (e.g. client features), so the read model the cache uses should be client features read model, the same you'd use if you don't go through the cache. Having a different read model for caching I think risks having different responses and can be a maintainability challenge.

Based on your comment maybe this should be called "client features read model" and stop using "client feature toggle store" (or client feature toggle store to delegate to this read model.

@sjaanus
Copy link
Contributor Author

sjaanus commented Dec 13, 2024

Personally, I don't like the fact of having a specific read model for the cache. The cache should cashe a read model for something (e.g. client features), so the read model the cache uses should be client features read model, the same you'd use if you don't go through the cache. Having a different read model for caching I think risks having different responses and can be a maintainability challenge.

Based on your comment maybe this should be called "client features read model" and stop using "client feature toggle store" (or client feature toggle store to delegate to this read model.

Yes, this has been long in our technical debt, that currently admin/features and client/features share the same store method. Although I do not want to touch the client/features currently when we are still experiment.
But yes in future, we should be using this same method for client/features also.

Copy link
Contributor

@FredrikOseberg FredrikOseberg left a comment

Choose a reason for hiding this comment

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

LGTM

@sjaanus sjaanus merged commit 63d2359 into main Dec 13, 2024
8 checks passed
@sjaanus sjaanus deleted the read-mo branch December 13, 2024 08:23
@FredrikOseberg
Copy link
Contributor

Personally, I don't like the fact of having a specific read model for the cache. The cache should cashe a read model for something (e.g. client features), so the read model the cache uses should be client features read model, the same you'd use if you don't go through the cache. Having a different read model for caching I think risks having different responses and can be a maintainability challenge.

Based on your comment maybe this should be called "client features read model" and stop using "client feature toggle store" (or client feature toggle store to delegate to this read model.

If you look at the client-feature-toggle-store it still contains a lot of technical debt from being a shared store with the admin API. This defeated the purpose of having a separate API because changing the store in the Admin API meant we risked breaking the client API.

We intend to use a feature flag to compare the output of the Delta API and the Polling API. Basically we will replay all the cache revisions on top of the base case and compare to the polling API. This will allow us to see that the output of the Delta API is correct over time and that there are no discrepancies. This allows us to find any edge cases without risking anything. Once we are certain we match the output 100% we can use implement this method in the store and share it for both endpoints.

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

Successfully merging this pull request may close these issues.

3 participants