Container Load Balancer - Thread Safe String Set #8559
Conversation
k8s-ci-robot
added
release-note-none
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: georgeedward2000 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: no
|
Hi @georgeedward2000. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
georgeedward2000
changed the title
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| other.mu.RLock() | ||
| defer other.mu.RUnlock() |
There was a problem hiding this comment.
Is this safe? I think it can deadlock if it will be called from both sets at the same time. We should use some property (memory address?) of each set to lock in the same order no matter which set calls equals.
There was a problem hiding this comment.
What do you mean? Multiple threads can hold RLocks at a time.
| } | ||
|
|
||
| for item := range s.set { | ||
| if _, exists := other.set[item]; !exists { |
There was a problem hiding this comment.
I think we need to ignore case here since we call it threadsafeignorecaseset.
| s := &ThreadSafeIgnoreCaseSet{ | ||
| set: make(map[string]struct{}), | ||
| } | ||
| s.mu.Lock() |
There was a problem hiding this comment.
What's the point here where a newly created item holds the lock?
| // ThreadSafeIgnoreCaseSet is a set of strings that is case-insensitive and thread-safe. | ||
| type ThreadSafeIgnoreCaseSet struct { | ||
| mu sync.RWMutex | ||
| set map[string]struct{} |
There was a problem hiding this comment.
can we reuse ignorecaseset here?
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces a thread-safe set implementation for strings, ensuring safe concurrent access and modifications. This enhancement is crucial for features that require high concurrency and data integrity, as it prevents race conditions and ensures consistent behavior across multiple goroutines.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: