Clear property caches on RPC request for props#2740
Conversation
Clear namespace, table, and system property cache entries when that particular configuration is being requested from the client Mitigation for apache#2739
- Alternate impl of PR apache#2740
|
I have no issues with the changes - but I created #2741 as an alternate so the casting to ZooPropStore is unnecessary |
* Rename PropCacheKey to PropStoreKey, since it's a first-class citizen of the PropStore interface * Add getCache() method to PropStore, because it is expected that PropStores are caching, so this makes it explicit * Use .getCache().remove() instead of casting to ZooPropStore and simulating a ZK change event to remove an item from the cache to force it to reload
|
I've made the changes here that I described in my #2741 (review) |
|
I'm not sure that PropStoreKey provides any benefit over PropCacheKey - I bounced around different variations, but maybe something that indicated that they are used for configurations (group of properties) - so something like ConfigKey or ConfigStoreKey,... Can the renaming be done outside of any other changes? The renaming is easy to check, and I think the other changes are being hidden by the rename changes. I lean towards providing methods to perform cache operations rather than returning the cache. If the cache is exposed, then it is much harder to reason what things are occurring through the prop store and what may be just manipulating the cache directly. |
To me, Prop and Config are basically synonymous in this context... both shorthand for "Configuration Properties". So, "ConfigKey" or "ConfigStoreKey" are basically the same in my head as "PropStoreKey", and "PropStoreKey" is consistent with the name of the "PropStore" (rather than "ConfigStore").
I suppose, but I'm probably not going to update this PR to do so until at least Tuesday.
The prop store already has to be written with no knowledge of what's going on inside the cache, since the cache itself can remove local copies at will. So, having something else have the ability to remove items from the cache is not a big deal. If you want to put cache-manipulation APIs on the propstore itself, then we probably don't need two interfaces and can merge PropStore and PropCache into a single CachingPropertyStore. |
|
See #2739 (comment) |
|
My issue is that if you return the cache, then callers can call any method - if a clear method is provided at the store level, then that bounds what the caller can do and would simplify changes at the cache layer. |
Just to be clear, we're talking about PropCache's API, not the internal Caffeine Cache object. So, we're already bounding the caller to what PropCache supports. I don't think it makes sense to turn PropStore API into a proxy for PropCache, with a subset of its APIs. Separation of responsibilities: the PropCache API is responsible for providing cache operations, the PropStore API is responsible for providing store operations. Neither are public API. There is no risk of exposing things to end users, because none of that matters here. It's just a question of how we're organizing internal code. I don't think it simplifies things to put cache operations on the PropStore API, rather than call them from the PropCache API. If we do that, there's no point in having separate interfaces to separate the responsibilities. I think doing that adds extra layers one has to trace code through when debugging internals, and makes things far more complex. If the operation is already available on PropCache, just make the PropCache visible. It can't get more simpler than that. |
dlmarion
left a comment
dlmarion
left a comment
There was a problem hiding this comment.
I think this will resolve the issues in PermissionsIT because only one ZK server is used with MAC. PermissionsIT.testArbitraryProperty() sets and gets a property value multiple times. The set call goes to the Manager and the get call goes to a TabletServer.
|
If we want this to resolve issues with more than one ZK server, then we could also add a ZK sync, but I think I'd prefer the PropStore itself do that when it refreshes in the background. |
|
I think this change will fix the tests, but come with a cost of refreshing the configuration information every time. I'm not sure what that actual cost is in terms of time on a larger cluster, but it fixes the test. I'm not suggesting that we add a ZK sync, my prior comments were a failed attempt at providing better consistency and a re-education on ZK's consistency guarantees. |
My goal here was to fix it for users as well as for tests. The performance impact of these changes should be minimal... because it only adds the cache expiration when a user explicitly calls public API to retrieve configuration. That's not how serves normally retrieve configuration... they do it directly through ZooKeeper. So, it should only affect specific use cases. However, depending on where we put a sync, that could have an impact on performance. If we put it in the prop cache loader, for example, it would sync even from normal cache expirations, and not user triggered ones. Putting a sync on these user-called methods wouldn't be too bad, though, I think. |
EdColeman
left a comment
EdColeman
left a comment
There was a problem hiding this comment.
I'm okay with merging this now to address the test stability. There maybe other things like adding a sync on update, but that can be handled via a separate PR. My other objections are more of a code style preference that is not worth delaying getting this in as is. We can have additional discussions to see if other changes are warranted.
4 participants
Clear namespace, table, and system property cache entries when that
particular configuration is being requested from the client
Mitigation for #2739