-
Notifications
You must be signed in to change notification settings - Fork 80
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
[server][common] Allow store config repo to fetch configs only when needed #1204
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking the following refactoring strategy, which I think will be safer:
- Keep the existing
refresh
calls as today. HelixReadOnlyStoreConfigRepository#refresh
will 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 fetch
function. - In
HelixReadOnlyStoreConfigRepository#getStoreConfig
function, 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. HelixReadOnlyStoreConfigRepository
would maintain astoreConfigMap
, but this map will be lazily populated byHelixReadOnlyStoreConfigRepository#getStoreConfig
function.
WDYT?
...ci-client/src/main/java/com/linkedin/davinci/repository/VeniceMetadataRepositoryBuilder.java
Show resolved
Hide resolved
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more comments and you need to fix the test failures...
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/helix/ZkStoreConfigAccessor.java
Show resolved
Hide resolved
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two more comments.
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Show resolved
Hide resolved
...enice-common/src/main/java/com/linkedin/venice/helix/HelixReadOnlyStoreConfigRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good overall now, and I left some minor improvement suggestions.
Summary
#1132 introduced additional ZK watch. The implementation of
HelixReadOnlyStoreConfigRepository
reads 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, it adds 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?