-
Notifications
You must be signed in to change notification settings - Fork 29
fix controller logic to prevent concurrent map reads and writes #28
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wlan0 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 |
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.
Minor question
controller/controller.go
Outdated
if op, ok := c.opMap[uuid]; ok { | ||
if op, ok := c.opMap.Load(uuid); ok { | ||
if _, ok := op.(updateOp); ok { | ||
return fmt.Errorf("cannot add already added object: %s", key) | ||
err := fmt.Errorf("cannot add already added object: %s", key) | ||
return err | ||
} | ||
} | ||
c.opMap[uuid] = addOp{ | ||
c.opMap.Store(uuid, addOp{ | ||
Object: d.Object, | ||
AddFunc: &add, | ||
Key: key, | ||
Indexer: indexer, | ||
} | ||
}) |
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.
Could we combine this by using c.opMap.LoadOrStore
/nit
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.
Sure
/lgtm |
Remove log levels and use klog v2
Remove log levels and use klog v2
updated README.md
There was a special case where under heavy load there was a concurrent map read and write. This fixes that