[server][common] Allow store config repo to fetch configs only when needed #1204
Conversation
adamxchen
changed the title
There was a problem hiding this comment.
I am thinking the following refactoring strategy, which I think will be safer:
- Keep the existing
refreshcalls as today. HelixReadOnlyStoreConfigRepository#refreshwill place a child-change listener as today, but it will only get a list of store names, not store configs.- In the child-change listener, whenever there is a child change, it will only mutate the maintained store list and of coz, for deleted stores, we will remove the corresponding entry from the local store config map and corresponding zk watch.
- We will only place a zk watch on store config node in
lazy fetchfunction. - In
HelixReadOnlyStoreConfigRepository#getStoreConfigfunction, if the requested store doesn't in the store list, returnOptional.empty()directly, and if it is present, fetch the store config on the fly and place the zk watch to monitor data change. HelixReadOnlyStoreConfigRepositorywould maintain astoreConfigMap, but this map will be lazily populated byHelixReadOnlyStoreConfigRepository#getStoreConfigfunction.
WDYT?
| @@ -147,7 +147,6 @@ private void initServerStoreAndSchemaRepository() { | |||
| adapter, | |||
| clusterConfig.getRefreshAttemptsForZkReconnect(), | |||
| clusterConfig.getRefreshIntervalForZkReconnectInMs()); | |||
| storeConfigRepo.refresh(); | |||
There was a problem hiding this comment.
I think we should still call refresh to get a list of stores, and please see my following comments.
There was a problem hiding this comment.
Thanks for the great suggestions! The core idea is only place watch when there's a need to get store config, and for other stores, we just cache them to know whether they are present or not. Will update it
| accessor.subscribeStoreConfigDataChangedListener(veniceStoreName, storeConfigChangedListener); | ||
| } | ||
| } | ||
|
|
||
| if (config != null) { | ||
| return Optional.of(config.cloneStoreConfig()); | ||
| } else { |
There was a problem hiding this comment.
There is already a branch for config being null. Shall we put the new logic here or remove it?
There was a problem hiding this comment.
After the new logic, the config can still be null or not-null so this branch still applies. We may use ternary operator to make it look nice tho
| config = accessor.getStoreConfig(veniceStoreName); | ||
| if (config != null) { | ||
| storeConfigMap.get().put(config.getStoreName(), config); | ||
| accessor.subscribeStoreConfigDataChangedListener(veniceStoreName, storeConfigChangedListener); |
There was a problem hiding this comment.
I wonder if we can unsubscribe when the store is deleted from this cluster; is it feasible with the current implementation? Does it know if the store is deleted?
There was a problem hiding this comment.
IIUC, we will need the StoreConfigAddedOrDeletedChangedListener for this and that's only used in refresh.. Let me see if I can incorporate Gaojie's suggestion to refactor this into next revision
Summary
#1132 introduced additional ZK watch. The implementation of
HelixReadOnlyStoreConfigRepositoryreads all store configs, regardless of which cluster, this creates an issue since it would increase the additional ZK watch bystore number x host name. In this PR,refreshcall is removed since we don't need all configs and add some logics to lazy fetch the store config and cache.How was this PR tested?
new unit test
Does this PR introduce any user-facing changes?