-
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
[Bug]: Concurrent caching & eviction param item value may cause cache dirty #33784
Closed
1 task done
Labels
kind/bug
Issues or changes related a bug
triage/accepted
Indicates an issue or PR is ready to be actively worked on.
Comments
congqixia
added a commit
to congqixia/milvus
that referenced
this issue
Jun 12, 2024
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>
sre-ci-robot
pushed a commit
that referenced
this issue
Jun 12, 2024
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>
congqixia
added a commit
to congqixia/milvus
that referenced
this issue
Jun 12, 2024
…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>
sre-ci-robot
pushed a commit
that referenced
this issue
Jun 13, 2024
…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>
yellow-shine
pushed a commit
to yellow-shine/milvus
that referenced
this issue
Jul 2, 2024
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
kind/bug
Issues or changes related a bug
triage/accepted
Indicates an issue or PR is ready to be actively worked on.
Is there an existing issue for this?
Environment
Current Behavior
Concurrent caching & eviction param item value may cause cache dirty
which may cause some unit test unstable:
TestChannelManager_BackgroundChannelChecker/test_enable_auto_balance
Expected Behavior
Config manager shall not cache dirty value
Steps To Reproduce
No response
Milvus Log
Anything else?
No response
The text was updated successfully, but these errors were encountered: