Skip to content

Add method to clear local store cache, update client service handler.#2741

Closed
EdColeman wants to merge 2 commits intoapache:mainfrom
EdColeman:add_zk_clear_event_to_propstore
Closed

Add method to clear local store cache, update client service handler.#2741
EdColeman wants to merge 2 commits intoapache:mainfrom
EdColeman:add_zk_clear_event_to_propstore

Conversation

@EdColeman
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I thought about doing this in my PR, but got stuck on naming, because it wasn't obvious in the PropStore interface that the implementation would even have a locally cached copy.

I've updated my PR #2740 with the changes I think would make the situation more explicit.

I'm not sure why, but both our PRs failed in GitHub Actions. My branch passed in a Jenkins instance, in just under 4 hours, which is about 15-20 minutes faster than normal. My guess is that it finished faster because fewer flakes were re-run.

* @param propCacheKey
* the prop cache key
*/
void clearLocal(PropCacheKey<?> propCacheKey);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This javadoc is really the first time it is explained anywhere in this interface that there's a locally cached copy of anything. Aside from the key being named "PropCacheKey", it's not really obvious that implementations of this interface will be doing any kind of caching. In fact, I think PropCacheKey should really be named PropStoreKey, because it's so tightly coupled to the PropStore interface, and a cache isn't necessarily implemented.

I think instead of this method, it would be good to have two changes:

  1. Rename PropCacheKey to PropStoreKey (and subclasses accordingly)
  2. Add a method to this interface called getCache() that returns an instance of the cache, to make it more obvious that it is expected for a PropStore implementation to use one

Then, instead of calling this clearLocal method, they could just do context.propStore().getCache().remove(propStoreKey)

@ctubbsii
Copy link
Copy Markdown
Member

This PR is basically a duplicate of what I was doing with #2740. Can we close this and collaborate / come to consensus there instead of having two open PRs trying to do the same basic thing?

@EdColeman
Copy link
Copy Markdown
Contributor Author

closing in favor of #2740

@EdColeman EdColeman closed this May 31, 2022
@EdColeman EdColeman deleted the add_zk_clear_event_to_propstore branch May 31, 2022 16:45
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

Successfully merging this pull request may close these issues.

3 participants