Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed a bug where initialPopulationCount should be based on the key length not list size in DeltaFIFO#Replace() #96797

Merged
merged 1 commit into from Dec 9, 2020
Merged

Fixed a bug where initialPopulationCount should be based on the key length not list size in DeltaFIFO#Replace() #96797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions staging/src/k8s.io/client-go/tools/cache/delta_fifo.go
View file
Open in desktop
Expand Up @@ -572,7 +572,7 @@ func (f *DeltaFIFO) Replace(list []interface{}, resourceVersion string) error {
f.populated = true
// While there shouldn't be any queued deletions in the initial
// population of the queue, it's better to be on the safe side.
f.initialPopulationCount = len(list) + queuedDeletions
f.initialPopulationCount = keys.Len() + queuedDeletions
}

return nil
Expand Down Expand Up @@ -602,7 +602,7 @@ func (f *DeltaFIFO) Replace(list []interface{}, resourceVersion string) error {

if !f.populated {
f.populated = true
f.initialPopulationCount = len(list) + queuedDeletions
f.initialPopulationCount = keys.Len() + queuedDeletions
}

return nil
Expand Down
9 changes: 9 additions & 0 deletions staging/src/k8s.io/client-go/tools/cache/delta_fifo_test.go
View file
Open in desktop
Expand Up @@ -633,6 +633,15 @@ func TestDeltaFIFO_HasSynced(t *testing.T) {
},
expectedSynced: true,
},
{
// This test case won't happen in practice since a Reflector, the only producer for delta_fifo today, always passes a complete snapshot consistent in time;
// there cannot be duplicate keys in the list or apiserver is broken.
actions: []func(f *DeltaFIFO){
func(f *DeltaFIFO) { f.Replace([]interface{}{mkFifoObj("a", 1), mkFifoObj("a", 2)}, "0") },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace always captures a complete snapshot consistent in time; there cannot be duplicate keys in the list or apiserver is broken.

...I guess maybe it can still be tested but please add a comment explaining that (and why) this is not testing a normal codepath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Added comment per suggestion. @lavalamp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording from @lavalamp , copied into the comment, seems a bit misdirected to me. Rather than saying "Replace always captures" --- which suggests that the responsibility for this behavior is inside Replace --- I suggest something more like "A Reflector always passes to Replace". It looks to me like the whole point of this PR is to move beyond the limited usage documented at

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

Lines 134 to 135 in e3de622

// DeltaFIFO is a producer-consumer queue, where a Reflector is
// intended to be the producer, and the consumer is whatever calls
("where a Reflector is intended to be the producer").

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah perhaps "expects" would have been a better word than "captures".

Having the comment start on the same line as the brace looks a little odd, I don't think we do that usually?

If you can fix these two tiny nits I can LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MikeSpreitzer and @lavalamp.

I rephrased the comments in the test to

This test case won't happen in practice since a Reflector, the only producer for delta_fifo today, always passes a complete snapshot consistent in time; there cannot be duplicate keys in the list or apiserver is broken.

Please let me know if this looks better.

func(f *DeltaFIFO) { Pop(f) },
},
expectedSynced: true,
},
}

for i, test := range tests {
Expand Down