DeltaFIFO cleanup #70801

Adirio · 2018-11-08T15:03:43Z

What type of PR is this?
/kind cleanup
/sig api-machinery

What this PR does / why we need it:
It does a clean up of some parts of the DeltaFIFO code:

A conditional that could never happen was being considered. This case checks for 0-length slices of Deltas. This slice is formed by adding a Delta to a previous slice, so it will always have at least one. After that the slice is deduped, but this method will never reduce the length to zero unless it was already zero. The behavior for zero length arrays is kept as a comment in case this can happen in the future.
EDIT: this change has been removed from this PR as a case where this could be possible was shown by @lavalamp. Instead some cleanup has been made in the same part: the else condition is not needed as delete is a no-op for non-existing keys.
A swallow copy of a slice is being done to instantly get the last element, thus making the copy useless. Issue [Question][client-go/tools/cache] DeltaFIFO.List() copy requirement #70751 tracks if a deep copy is needed, what would require a fix, but this PR keeps the current behavior.
Return the closed attribute directly instead of checking it and then returning a constant.
Reorder a counter decrease to stay near the element extraction

Special notes for your reviewer:
The first and last ones are mostly syntactic sugar, but the middle one should slightly improve the performance.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2018-11-08T15:03:49Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2018-11-08T15:03:51Z

Hi @Adirio. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

Adirio · 2018-11-08T15:25:11Z

Following @k8s-ci-robot suggestion:

/assign @lavalamp

roycaihw · 2018-11-08T21:30:03Z

/cc

roycaihw · 2018-11-13T02:32:36Z

/ok-to-test

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

lavalamp · 2018-11-13T05:43:19Z

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

-		// map).
-		delete(f.items, id)
+	// newDeltas will always have at least one delta, if there is a case where
+	// it may have no elements, delete f.items[id] (f.queue will ignore elements


This comment is confusingly worded, as it's talking about a case that is no longer handled here without making that clear.

Will rephrase if this change is accepted.

I recommend just removing this change and sending it in another PR; the others I can LGTM now but we're probably going to need a few iterations on the comment text here.

lavalamp · 2018-11-13T05:45:29Z

sorry, I started reviewing this a while ago and got distracted.

I don't agree with the first change (I'm not arguing the existing code is optimal--it should at least be tested). The rest are fine.

Adirio · 2018-11-13T07:51:52Z

I don't agree with the first change (I'm not arguing the existing code is optimal--it should at least be tested). The rest are fine.

If you still think that we should test for empty slices, I will modify this PR to only include the two last points.

Remove non-needed else condition Remove non-needed swallow copy Simplify return for IsClosed() Keep amount decrement next to element extraction from the queue Signed-off-by: Adrián Orive <adrian.orive.oneca@gmail.com>

Adirio · 2018-11-14T12:30:01Z

@lavalamp Everything ready and checks passing (except the tide one as it requires LGTM, aproval and priority labels)

lavalamp · 2018-11-14T23:18:38Z

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

 	if len(newDeltas) > 0 {
-		if !exists {
+		if _, exists := f.items[id]; !exists {


I probably wouldn't have made these changes at all but I guess I don't object :)

(meaning, just these two--I'm a fan of the others)

This change will mean nothing for most of the cases, as for now dedup will not yield an empty slice, but should optimize the path for empty slices. But I must agree is mostly a cosmetic change.

lavalamp · 2018-11-14T23:19:25Z

/lgtm
/approve
/milestone v1.13

k8s-ci-robot · 2018-11-14T23:19:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, lavalamp

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:

~~staging/src/k8s.io/client-go/tools/cache/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Adirio · 2018-11-14T23:30:56Z

I think it is just missing a priority now. Which one should I use?

lavalamp · 2018-11-15T00:10:46Z

/priority important-soon

…

On Wed, Nov 14, 2018 at 3:32 PM Adrián ***@***.***> wrote: I think it is just missing a priority now. Which one should I use? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70801 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAngltCvThMQD6S2vQRP3m5S2VXq0DPsks5uvKf5gaJpZM4YU2Vp> .

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2018

k8s-ci-robot requested review from david-mcmahon and dims November 8, 2018 15:03

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 8, 2018

k8s-ci-robot assigned lavalamp Nov 8, 2018

k8s-ci-robot requested a review from roycaihw November 8, 2018 21:30

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 13, 2018

lavalamp reviewed Nov 13, 2018

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go Outdated Show resolved Hide resolved

lavalamp reviewed Nov 13, 2018

View reviewed changes

DeltaFIFO cleanup

0b16c43

Remove non-needed else condition Remove non-needed swallow copy Simplify return for IsClosed() Keep amount decrement next to element extraction from the queue Signed-off-by: Adrián Orive <adrian.orive.oneca@gmail.com>

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 14, 2018

lavalamp reviewed Nov 14, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2018

k8s-ci-robot added this to the v1.13 milestone Nov 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2018

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 15, 2018

k8s-ci-robot merged commit 4fb368e into kubernetes:master Nov 15, 2018

Adirio deleted the deltafifo-cleanup branch April 29, 2019 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DeltaFIFO cleanup #70801

DeltaFIFO cleanup #70801

Adirio commented Nov 8, 2018 •

edited

k8s-ci-robot commented Nov 8, 2018

k8s-ci-robot commented Nov 8, 2018

Adirio commented Nov 8, 2018 •

edited

roycaihw commented Nov 8, 2018

roycaihw commented Nov 13, 2018

lavalamp Nov 13, 2018

Adirio Nov 13, 2018

lavalamp Nov 13, 2018

lavalamp commented Nov 13, 2018

Adirio commented Nov 13, 2018

Adirio commented Nov 14, 2018

lavalamp Nov 14, 2018

lavalamp Nov 14, 2018

Adirio Nov 14, 2018

lavalamp commented Nov 14, 2018

k8s-ci-robot commented Nov 14, 2018

Adirio commented Nov 14, 2018

lavalamp commented Nov 15, 2018 via email

DeltaFIFO cleanup #70801

DeltaFIFO cleanup #70801

Conversation

Adirio commented Nov 8, 2018 • edited

k8s-ci-robot commented Nov 8, 2018

k8s-ci-robot commented Nov 8, 2018

Adirio commented Nov 8, 2018 • edited

roycaihw commented Nov 8, 2018

roycaihw commented Nov 13, 2018

lavalamp Nov 13, 2018

Choose a reason for hiding this comment

Adirio Nov 13, 2018

Choose a reason for hiding this comment

lavalamp Nov 13, 2018

Choose a reason for hiding this comment

lavalamp commented Nov 13, 2018

Adirio commented Nov 13, 2018

Adirio commented Nov 14, 2018

lavalamp Nov 14, 2018

Choose a reason for hiding this comment

lavalamp Nov 14, 2018

Choose a reason for hiding this comment

Adirio Nov 14, 2018

Choose a reason for hiding this comment

lavalamp commented Nov 14, 2018

k8s-ci-robot commented Nov 14, 2018

Adirio commented Nov 14, 2018

lavalamp commented Nov 15, 2018 via email

Adirio commented Nov 8, 2018 •

edited

Adirio commented Nov 8, 2018 •

edited