[client-go #1415] Embed proper interface in TransformingStore to ensure DeltaFIFO and RealFIFO are implementing it #133263
Conversation
…re to ensure DeltaFIFO and RealFIFO are implementing it Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
|
Hi @valerian-roche. Thanks for your PR. I'm waiting for a kubernetes 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: valerian-roche 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 |
|
/ok-to-test |
I'm pretty sure RealFIFO is the structure actually used in informers by default |
Both are still in the code, not sure which one is used by default now. I can set the test to run with both if needed |
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
Updated the test to run against both |
| metav1.InitialEventsAnnotationKey: "true", | ||
| type testReflectorStore interface { | ||
| ReflectorStore | ||
| list() []interface{} |
There was a problem hiding this comment.
can this test introspect the items in the stores without adding methods to the implementations in the _test files?
There was a problem hiding this comment.
Yes let me rewrite those - updated.
This complexity the test a bit but this can be cleaned once DeltaFIFO is fully removed
|
/retest |
|
@valerian-roche: 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. 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. I understand the commands that are listed here. |
|
/triage accepted |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR is a follow-up to #131799 to improve the memory utilization of informers using transformers.
In #131799 I added a new interface to match stores providing transformers in order to reuse them. The PR was extracted from our fork but a change to embed the
Storeinterface in theTransformingStore(instead ofReflectorStore) mistakenly broke the interface matching. The tests did not catch it as they usefakeStorewhich is an implementation ofStore.This PR updates the embedded interface to the proper one, and add interface enforcement for
DeltaFIFOandRealFIFO. It also updates the test to directly useDeltaFIFO(as this is the structure actually used in the informers).While adding the tests, I could not reproduce the issue as
DeltaFIFOdoes implementStorethrough test files only. This is in my opinion a very poor behavior, as in this case tests would have passed without errors and failed again at runtime. It was only discovered while running the new tests against the old behavior and noting an unexpected success.Which issue(s) this PR is related to:
Fixes kubernetes/client-go#1415
Special notes for your reviewer:
The only functional change of the PR is the replacement of the embedded interface in
TransformingStore. The rest of the PR is for maintainability and updated testsDoes this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: