change where transformers are called #116623
Conversation
k8s-ci-robot
added
size/M
| // | ||
| // The most common usage pattern is to clean-up some parts of the object to | ||
| // reduce component memory usage if a given component doesn't care about them. | ||
| // given controller doesn't care for them. |
There was a problem hiding this comment.
heh, so was the original.
I updated all the comments to reflect my current beliefs about how it behaves now. It needs tests to verify.
|
Looks correct to me at first pass, but I will defer to someone who has looked at deltafifo more recently. |
k8s-ci-robot
added
release-note-none
|
/milestone v1.27 |
| // | ||
| // New in v1.27: In such cases, the contained object will already have gone | ||
| // through the transform object separately (when it was added / updated prior | ||
| // to the delete), so the TransformFunc can likely safely ignore such objects. |
There was a problem hiding this comment.
Nice, this comment is very good!
Should we maybe rephrase this to state that "ignore" means returning the input object to be 100% clear?
There was a problem hiding this comment.
Yes, I will update. I'm adding a test now
There was a problem hiding this comment.
Nice, thanks! Also feel free to take some logic from #116432 as well, or we can do that as a follow up after this, thats up to you
|
/assign |
|
What do you think about backporting this PR together with #115620? |
k8s-ci-robot
added
size/L
|
I ran unit tests for an hour with stress and race and no problems found.
Yeah they need to be backported together. But right now I'm worried about not blocking the release of 1.27 :) /milestone v1.27 |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
There was a problem hiding this comment.
@odinuge do you think you can rebase #116432 on top of this so we can use the test you wrote there? I think after this PR there's a case that you shouldn't have to handle any more (tombstone object with untransformed inner object).
Sweet. Did that now, and the tests passed the race detector locally without any problems. I'll clean that PR up when this is merged and I have some more time maybe tomorrow.
Overall I like this approach, and the code looks good, together with a very descriptive set of godocs. Thanks!
/lgtm
Adding a hold in case others want to look, but anyone feel free to remove it!
/hold
k8s-ci-robot
added
do-not-merge/hold
|
LGTM label has been added. Git tree hash: 9ab2ed5512286f02d1130fef04875873b52067ea
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, odinuge 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 |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
For posterity - this LGTM too Thanks for fixing this Daniel! |
What type of PR is this?
/kind bug
/kind regression
What this PR does / why we need it:
Code changes needed for the problem discovered in #116432
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.: