Skip to content

Make AbstractTrait::getId and $namespace protected. #23960

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

Closed
wants to merge 1 commit into from

Conversation

hcomnetworkers
Copy link

@hcomnetworkers hcomnetworkers commented Aug 23, 2017

Q A
Branch? 3.3
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

This PR allows to extend the getId-method of the caching classes.

Use case: Incrementing the namespace upon a clear()-call instead of flushing all cache instances with all namespaces (e.g. with Memcached).

@stof
Copy link
Member

stof commented Aug 23, 2017

Well, this method is not a supported extension point currently. I'm not sure we want to make it a supported inheritance extension point for people extending our cache implementations

@stof
Copy link
Member

stof commented Aug 23, 2017

and anyway, doing it in 3.3 is a no-go. Adding new inheritance-based extension point is considered a new feature.

@nicolas-grekas
Copy link
Member

I agree with @stof
Incrementing the namespace is not a supported strategy because it requires a backend round-trip to fetch its current version.
Instead, just change the namespace.

@hcomnetworkers
Copy link
Author

$namespace is also private and there is no setter.

@hcomnetworkers
Copy link
Author

I can achieve the same result by extending all the protected methods and changing the id there, but since there is already getId-method that seems a bit bulky.

@nicolas-grekas
Copy link
Member

So, you need instant clearing? Did you measure it as slow on your use case? By how much?

@hcomnetworkers
Copy link
Author

I want to clear a namespaced memcached instance and I expect all other memcached instances not to be cleared afterwards.
Memcached does not support clearing by prefix or regex like APC or Redis.
And for clearing a namespaced cache, incrementing the namespace is good solution.

@nicolas-grekas
Copy link
Member

OK, got it. I suggest considering the issue as a bug, and implementing this strategy in the existing MemcachedTrait: when a non empty namespace is used, we do versioning. The extra round trip is the only way to solve the issue anyway.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 24, 2017

Thank you for raising the point @hcomnetworkers
I implemented by-versioning clearing in #23969 as an option for drivers (since RedisCluster also had a similar issue).
I'd be happy to have your review on that PR.
Closing this PR meanwhile. Thanks again.

fabpot added a commit that referenced this pull request Aug 31, 2017
…pport clearing by keys (nicolas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[Cache] Use namespace versioning for backends that dont support clearing by keys

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23960
| License       | MIT
| Doc PR        | -

Commits
-------

f8a7518 [Cache] Use namespace versioning for backends that dont support clearing by keys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants