From 56159f258ca380600b0bc08b2e99cbc745db3560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Tyczy=C5=84ski?= Date: Wed, 30 Mar 2022 08:52:10 +0200 Subject: [PATCH 1/2] Add test for indexer with multiple values --- .../tools/cache/thread_safe_store_test.go | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/staging/src/k8s.io/client-go/tools/cache/thread_safe_store_test.go b/staging/src/k8s.io/client-go/tools/cache/thread_safe_store_test.go index 591ff37277d..c4cbd4d08f9 100644 --- a/staging/src/k8s.io/client-go/tools/cache/thread_safe_store_test.go +++ b/staging/src/k8s.io/client-go/tools/cache/thread_safe_store_test.go @@ -18,7 +18,11 @@ package cache import ( "fmt" + "strings" "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" ) func TestThreadSafeStoreDeleteRemovesEmptySetsFromIndex(t *testing.T) { @@ -92,6 +96,75 @@ func TestThreadSafeStoreAddKeepsNonEmptySetPostDeleteFromIndex(t *testing.T) { } } +func TestThreadSafeStoreIndexingFunctionsWithMultipleValues(t *testing.T) { + testIndexer := "testIndexer" + + indexers := Indexers{ + testIndexer: func(obj interface{}) ([]string, error) { + return strings.Split(obj.(string), ","), nil + }, + } + + indices := Indices{} + store := NewThreadSafeStore(indexers, indices).(*threadSafeMap) + + store.Add("key1", "foo") + store.Add("key2", "bar") + + assert := assert.New(t) + + compare := func(key string, expected []string) error { + values := store.indices[testIndexer][key].List() + if cmp.Equal(values, expected) { + return nil + } + return fmt.Errorf("unexpected index for key %s, diff=%s", key, cmp.Diff(values, expected)) + } + + assert.NoError(compare("foo", []string{"key1"})) + assert.NoError(compare("bar", []string{"key2"})) + + store.Update("key2", "foo,bar") + + assert.NoError(compare("foo", []string{"key1", "key2"})) + assert.NoError(compare("bar", []string{"key2"})) + + store.Update("key1", "foo,bar") + + assert.NoError(compare("foo", []string{"key1", "key2"})) + assert.NoError(compare("bar", []string{"key1", "key2"})) + + store.Add("key3", "foo,bar,baz") + + assert.NoError(compare("foo", []string{"key1", "key2", "key3"})) + assert.NoError(compare("bar", []string{"key1", "key2", "key3"})) + assert.NoError(compare("baz", []string{"key3"})) + + store.Update("key1", "foo") + + assert.NoError(compare("foo", []string{"key1", "key2", "key3"})) + assert.NoError(compare("bar", []string{"key2", "key3"})) + assert.NoError(compare("baz", []string{"key3"})) + + store.Update("key2", "bar") + + assert.NoError(compare("foo", []string{"key1", "key3"})) + assert.NoError(compare("bar", []string{"key2", "key3"})) + assert.NoError(compare("baz", []string{"key3"})) + + store.Delete("key1") + + assert.NoError(compare("foo", []string{"key3"})) + assert.NoError(compare("bar", []string{"key2", "key3"})) + assert.NoError(compare("baz", []string{"key3"})) + + store.Delete("key3") + + assert.NoError(compare("foo", []string{})) + assert.NoError(compare("bar", []string{"key2"})) + assert.NoError(compare("baz", []string{})) +} + func BenchmarkIndexer(b *testing.B) { testIndexer := "testIndexer" From cbbb5f70a47644f9830073d9d0329bf247a328a1 Mon Sep 17 00:00:00 2001 From: Michael Bolot Date: Tue, 29 Mar 2022 12:35:13 -0500 Subject: [PATCH 2/2] Addresses the issue which caused #109115 --- .../client-go/tools/cache/thread_safe_store.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go b/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go index 2ab926825dc..6d58c0d690d 100644 --- a/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go +++ b/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go @@ -280,18 +280,15 @@ func (c *threadSafeMap) updateIndices(oldObj interface{}, newObj interface{}, ke c.indices[name] = index } + if len(indexValues) == 1 && len(oldIndexValues) == 1 && indexValues[0] == oldIndexValues[0] { + // We optimize for the most common case where indexFunc returns a single value which has not been changed + continue + } + for _, value := range oldIndexValues { - // We optimize for the most common case where indexFunc returns a single value. - if len(indexValues) == 1 && value == indexValues[0] { - continue - } c.deleteKeyFromIndex(key, value, index) } for _, value := range indexValues { - // We optimize for the most common case where indexFunc returns a single value. - if len(oldIndexValues) == 1 && value == oldIndexValues[0] { - continue - } c.addKeyToIndex(key, value, index) } }