-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix: Compare config value then swap when caching param value #33785
fix: Compare config value then swap when caching param value #33785
Conversation
See also milvus-io#33784 This PR change the behavior of `SetCacheValue` of config manager: - Use mutex and map instead of concurrent map for `configCache` - Compare config raw value before set cache value With this implementation, concurrent caching & eviction shall always have current output: |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 |CAS OK | |old value|old value| |t2 | |update |new value|old value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 | |update |new value|null | |t2 |CAS fail| |old value|null | |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 |CAS OK | |new value|new value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 | |eviction|new value|null | |t3 |CAS OK | |new value|new value| |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 | |eviction|new value|null | |t2 |get | |new value|null | |t3 |CAS OK | |new value|new value| Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33785 +/- ##
=======================================
Coverage 80.94% 80.95%
=======================================
Files 1058 1058
Lines 135119 135200 +81
=======================================
+ Hits 109377 109446 +69
- Misses 21555 21575 +20
+ Partials 4187 4179 -8
|
…io#33785) See also milvus-io#33784 This PR change the behavior of `SetCacheValue` of config manager: - Use mutex and map instead of concurrent map for `configCache` - Compare config raw value before set cache value With this implementation, concurrent caching & eviction shall always have current output: |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 |CAS OK | |old value|old value| |t2 | |update |new value|old value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 | |update |new value|null | |t2 |CAS fail| |old value|null | |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 |CAS OK | |new value|new value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 | |eviction|new value|null | |t3 |CAS OK | |new value|new value| |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 | |eviction|new value|null | |t2 |get | |new value|null | |t3 |CAS OK | |new value|new value| Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
…33785) (#33797) Cherry-pick from master pr: #33785 See also #33784 This PR change the behavior of `SetCacheValue` of config manager: - Use mutex and map instead of concurrent map for `configCache` - Compare config raw value before set cache value With this implementation, concurrent caching & eviction shall always have current output: |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 |CAS OK | |old value|old value| |t2 | |update |new value|old value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 | |update |new value|null | |t2 |CAS fail| |old value|null | |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 |CAS OK | |new value|new value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 | |eviction|new value|null | |t3 |CAS OK | |new value|new value| |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 | |eviction|new value|null | |t2 |get | |new value|null | |t3 |CAS OK | |new value|new value| --------- Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
…io#33785) See also milvus-io#33784 This PR change the behavior of `SetCacheValue` of config manager: - Use mutex and map instead of concurrent map for `configCache` - Compare config raw value before set cache value With this implementation, concurrent caching & eviction shall always have current output: |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 |CAS OK | |old value|old value| |t2 | |update |new value|old value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 |get | |old value|null | |t1 | |update |new value|null | |t2 |CAS fail| |old value|null | |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 |CAS OK | |new value|new value| |t3 | |eviction|new value|null | |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 |get | |new value|null | |t2 | |eviction|new value|null | |t3 |CAS OK | |new value|new value| |time|caching |eviction|config |cached | |----|--------|------- |---------|---------| |t0 | |update |new value|null | |t1 | |eviction|new value|null | |t2 |get | |new value|null | |t3 |CAS OK | |new value|new value| Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
See also milvus-io#33785 When config item is not present in paramtable, CAS fails due to GetConfig returns error. This PR make this returned err instance of ErrKeyNotFound and check error type in \`CASCachedValue\` methods. Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
See also milvus-io#33785 When config item is not present in paramtable, CAS fails due to GetConfig returns error. This PR make this returned err instance of ErrKeyNotFound and check error type in \`CASCachedValue\` methods. Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
See also #33785 When config item is not present in paramtable, CAS fails due to GetConfig returns error. This PR make this returned err instance of ErrKeyNotFound and check error type in \`CASCachedValue\` methods. Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
See also #33784
This PR change the behavior of
SetCacheValue
of config manager:configCache
With this implementation, concurrent caching & eviction shall always have current output: