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

Make Cache in AssignmentFilter configurable #24

Closed
ruudk opened this issue Sep 5, 2024 · 8 comments
Closed

Make Cache in AssignmentFilter configurable #24

ruudk opened this issue Sep 5, 2024 · 8 comments

Comments

@ruudk
Copy link

ruudk commented Sep 5, 2024

As opposed to other languages , PHP is stateless. That means that keeping things inside an in memory cache will be cleared at the end of the request. That means that the LRUCache implementation in AssignmentFilter has no value.

The Cache should be made configurable.
While doing that, it's best to allow any PSR-6 cache.

The LRUCache can be dropped and replaced with an ArrayAdapter cache.

https://symfony.com/doc/current/components/cache.html
https://github.com/amplitude/experiment-php-server/blob/main/src/Assignment/LRUCache.php
https://github.com/amplitude/experiment-php-server/blob/main/src/Assignment/AssignmentFilter.php

Currently, we cannot use assignments because we need to make sure the cache is stateful.
It's impossible to define a custom AssignmentFilter. It's not an interface, so we have to extend the existing AssignmentFilter.
But then it's not possible to easily use it because this is initialized here:

new AssignmentFilter($config->cacheCapacity),

Another solution, as opposed to making the Cache configurable, is to make AssignmentFilter an interface, with a DefaultAssignmentFilter implementation that can be swapped.

/cc @bgiori

@ruudk
Copy link
Author

ruudk commented Sep 5, 2024

I think one solution could be to implement the filtering in the AssignmentTrackingProvider. But I feel that's doing the work twice.

@tyiuhc
Copy link
Collaborator

tyiuhc commented Sep 9, 2024

@ruudk Many thanks for raising this issue. This has been prioritized and we are working to address it.

@tyiuhc
Copy link
Collaborator

tyiuhc commented Sep 13, 2024

@ruudk, you can now configure a custom AssignmentFilter in v1.2.0. Please let us know if this is not working as expected, thanks!

@ruudk
Copy link
Author

ruudk commented Sep 14, 2024

Too bad I didn't have time to look at this and it's already tagged. But wouldn't it have been better to make the cache inside the default filter configurable? That way I don't have to create my own filter.

@tyiuhc
Copy link
Collaborator

tyiuhc commented Sep 14, 2024

@ruudk To use a custom cache with the default filter, you can create a DefaultAssignmentFilter with your custom cache, then use that as your filter in AssignmentConfig.

We wanted to allow additional flexibility in determining whether an assignment should be tracked. Hope that helps with your use case.

@ruudk
Copy link
Author

ruudk commented Sep 14, 2024

I understand. But given that much can be configured using a ConfigBuilder, I figured that allowing me to configure the cache there, would make it even easier 😁

@tyiuhc tyiuhc closed this as completed Sep 16, 2024
@ruudk
Copy link
Author

ruudk commented Sep 17, 2024

@tyiuhc It's not possible for me to use 1.2.0 because it's locked to symfony/cache ^5.

Could you please widen this to allow all the supported versions? https://symfony.com/releases

@tyiuhc tyiuhc reopened this Sep 17, 2024
@tyiuhc
Copy link
Collaborator

tyiuhc commented Sep 17, 2024

@ruudk Apologies for the inconvenience, v1.2.1 will allow additional versions of symfony/cache

@ruudk ruudk closed this as completed Nov 11, 2024
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

No branches or pull requests

2 participants