✨ POC for a read-your-own-write client#3472
✨ POC for a read-your-own-write client#3472alvaroaleman wants to merge 18 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
k8s-ci-robot
added
the
cncf-cla: yes
|
@alvaroaleman: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
| return fmt.Errorf("failed to get GVK for list %T: %w", list, err) | ||
| } | ||
|
|
||
| keys := c.lockedKeysByGVK.getOrCreate(gvk).allValues() |
There was a problem hiding this comment.
gvk.Kind = strings.TrimSuffix(gvk.Kind, "List")
| return c.clusterCache.AddRequiredDeleteForObject(obj) | ||
| } else if cache, ok := c.namespaceToCache[ns]; ok { | ||
| return cache.AddRequiredDeleteForObject(obj) | ||
| } |
There was a problem hiding this comment.
Looks like not handle global cache namespaceToCache[metav1.NamespaceAll]
| if ctx.Err() != nil { | ||
| break | ||
| } | ||
| h.pendingDeletesCond.Wait() |
There was a problem hiding this comment.
Since Wait() only wakes on Broadcast(), and Broadcast() only happens when new informer events arrive, the goroutine will leak
There was a problem hiding this comment.
Yes I am aware, but I think that's acceptable - If someone started waiting, the assumption is that an event will come so this won't be blocked for long
| if ctx.Err() != nil { | ||
| break | ||
| } | ||
| h.rvCond.Wait() |
There was a problem hiding this comment.
Since Wait() only wakes on Broadcast(), and Broadcast() only happens when new informer events arrive, the goroutine will leak
| @@ -0,0 +1,257 @@ | |||
| package client_test | |||
There was a problem hiding this comment.
Please double check (eventually, I know this is just a POC for now) that we have coverage that read-your-own write also works correctly with the multi namespace cache
| cache cache | ||
|
|
||
| // lockedKeysByGVK maps gvk -> key -> keyLocker | ||
| lockedKeysByGVK threadSafeMap[schema.GroupVersionKind, *threadSafeMap[types.NamespacedName, *keyLocker]] |
There was a problem hiding this comment.
If I saw correctly we only add but never remove from this map. While it doesn't take a lot of memory I wonder if this adds up
(didn't thoroughly review the entire PR, so I might have missed the cleanup)
4 participants
Related to #3320
/hold