From 36878e5e2bdea6f035eac6dd10c538ccc5585ade Mon Sep 17 00:00:00 2001 From: Matthew Keeler Date: Fri, 20 Dec 2024 11:29:08 -0500 Subject: [PATCH] fix: Ignore model version when applying FDv2 updates --- internal/memorystorev2/memory_store.go | 39 +---- .../memory_store_benchmark_test.go | 12 +- internal/memorystorev2/memory_store_test.go | 155 ++++++------------ 3 files changed, 65 insertions(+), 141 deletions(-) diff --git a/internal/memorystorev2/memory_store.go b/internal/memorystorev2/memory_store.go index 25b432de..3f42401f 100644 --- a/internal/memorystorev2/memory_store.go +++ b/internal/memorystorev2/memory_store.go @@ -55,28 +55,16 @@ func (s *Store) SetBasis(allData []ldstoretypes.Collection) { } // ApplyDelta applies a delta update to the store. ApplyDelta should not be called until -// SetBasis has been called at least once. The return value indicates, for each DataKind -// present in the delta, whether the item in the delta was actually updated or not. -// -// An item is updated only if the version of the item in the delta is greater than the version -// in the store, or it wasn't already present. -func (s *Store) ApplyDelta(allData []ldstoretypes.Collection) map[ldstoretypes.DataKind]map[string]bool { - updatedMap := make(map[ldstoretypes.DataKind]map[string]bool) - +// SetBasis has been called at least once. +func (s *Store) ApplyDelta(allData []ldstoretypes.Collection) { s.Lock() defer s.Unlock() for _, coll := range allData { for _, item := range coll.Items { - updated := s.upsert(coll.Kind, item.Key, item.Item) - if updatedMap[coll.Kind] == nil { - updatedMap[coll.Kind] = make(map[string]bool) - } - updatedMap[coll.Kind][item.Key] = updated + s.upsert(coll.Kind, item.Key, item.Item) } } - - return updatedMap } // Get retrieves an item of the specified kind from the store. If the item is not found, then @@ -139,27 +127,12 @@ func (s *Store) GetAllKinds() []ldstoretypes.Collection { func (s *Store) upsert( kind ldstoretypes.DataKind, key string, - newItem ldstoretypes.ItemDescriptor) bool { - var coll map[string]ldstoretypes.ItemDescriptor - var ok bool - shouldUpdate := true - updated := false - if coll, ok = s.data[kind]; ok { - if item, ok := coll[key]; ok { - if item.Version >= newItem.Version { - shouldUpdate = false - } - } + newItem ldstoretypes.ItemDescriptor) { + if coll, ok := s.data[kind]; ok { + coll[key] = newItem } else { s.data[kind] = map[string]ldstoretypes.ItemDescriptor{key: newItem} - shouldUpdate = false // because we already initialized the map with the new item - updated = true - } - if shouldUpdate { - coll[key] = newItem - updated = true } - return updated } // IsInitialized returns true if the store has ever been initialized with a basis. diff --git a/internal/memorystorev2/memory_store_benchmark_test.go b/internal/memorystorev2/memory_store_benchmark_test.go index a1644bbc..4a46d1e1 100644 --- a/internal/memorystorev2/memory_store_benchmark_test.go +++ b/internal/memorystorev2/memory_store_benchmark_test.go @@ -203,7 +203,7 @@ func BenchmarkInMemoryStoreUpsertExistingFlagSuccess(b *testing.B) { benchmarkInMemoryStore(b, inMemoryStoreBenchmarkCases, nil, func(env *inMemoryStoreBenchmarkEnv, bc inMemoryStoreBenchmarkCase) { env.targetFlagCopy.Version++ delta := makeCollections(dataKind, env.targetFlagKey, sharedtest.FlagDescriptor(*env.targetFlagCopy)) - updates = env.store.ApplyDelta(delta) + env.store.ApplyDelta(delta) }) } @@ -212,7 +212,7 @@ func BenchmarkInMemoryStoreUpsertExistingFlagFailure(b *testing.B) { benchmarkInMemoryStore(b, inMemoryStoreBenchmarkCases, nil, func(env *inMemoryStoreBenchmarkEnv, bc inMemoryStoreBenchmarkCase) { env.targetFlagCopy.Version-- delta := makeCollections(dataKind, env.targetFlagKey, sharedtest.FlagDescriptor(*env.targetFlagCopy)) - updates = env.store.ApplyDelta(delta) + env.store.ApplyDelta(delta) }) } @@ -221,7 +221,7 @@ func BenchmarkInMemoryStoreUpsertNewFlag(b *testing.B) { benchmarkInMemoryStore(b, inMemoryStoreBenchmarkCases, nil, func(env *inMemoryStoreBenchmarkEnv, bc inMemoryStoreBenchmarkCase) { env.targetFlagCopy.Key = env.unknownKey delta := makeCollections(dataKind, env.unknownKey, sharedtest.FlagDescriptor(*env.targetFlagCopy)) - updates = env.store.ApplyDelta(delta) + env.store.ApplyDelta(delta) }) } @@ -230,7 +230,7 @@ func BenchmarkInMemoryStoreUpsertExistingSegmentSuccess(b *testing.B) { benchmarkInMemoryStore(b, inMemoryStoreBenchmarkCases, nil, func(env *inMemoryStoreBenchmarkEnv, bc inMemoryStoreBenchmarkCase) { env.targetSegmentCopy.Version++ delta := makeCollections(dataKind, env.targetSegmentKey, sharedtest.SegmentDescriptor(*env.targetSegmentCopy)) - updates = env.store.ApplyDelta(delta) + env.store.ApplyDelta(delta) }) } @@ -239,7 +239,7 @@ func BenchmarkInMemoryStoreUpsertExistingSegmentFailure(b *testing.B) { benchmarkInMemoryStore(b, inMemoryStoreBenchmarkCases, nil, func(env *inMemoryStoreBenchmarkEnv, bc inMemoryStoreBenchmarkCase) { env.targetSegmentCopy.Version-- delta := makeCollections(dataKind, env.targetSegmentKey, sharedtest.SegmentDescriptor(*env.targetSegmentCopy)) - updates = env.store.ApplyDelta(delta) + env.store.ApplyDelta(delta) }) } @@ -248,6 +248,6 @@ func BenchmarkInMemoryStoreUpsertNewSegment(b *testing.B) { benchmarkInMemoryStore(b, inMemoryStoreBenchmarkCases, nil, func(env *inMemoryStoreBenchmarkEnv, bc inMemoryStoreBenchmarkCase) { env.targetSegmentCopy.Key = env.unknownKey delta := makeCollections(dataKind, env.unknownKey, sharedtest.SegmentDescriptor(*env.targetSegmentCopy)) - updates = env.store.ApplyDelta(delta) + env.store.ApplyDelta(delta) }) } diff --git a/internal/memorystorev2/memory_store_test.go b/internal/memorystorev2/memory_store_test.go index b47f0ffd..94287d86 100644 --- a/internal/memorystorev2/memory_store_test.go +++ b/internal/memorystorev2/memory_store_test.go @@ -249,116 +249,67 @@ func (k unknownDataKind) Deserialize(data []byte) (ldstoretypes.ItemDescriptor, func testApplyDelta(t *testing.T) { forAllDataKinds(t, func(t *testing.T, kind ldstoretypes.DataKind, makeItem collectionItemCreator, deleteItem collectionItemDeleter) { t.Run("upserts", func(t *testing.T) { - t.Run("newer version", func(t *testing.T) { - store := makeMemoryStore() - store.SetBasis(sharedtest.NewDataSetBuilder().Build()) - - _, collection1 := makeItem("key", 10, false) - - updates := store.ApplyDelta(collection1) - assert.True(t, updates[kind]["key"]) - - item1a, collection1a := makeItem("key", 11, true) - - updates = store.ApplyDelta(collection1a) - assert.True(t, updates[kind]["key"]) - - result, err := store.Get(kind, "key") - require.NoError(t, err) - assert.Equal(t, item1a, result) - - }) - - t.Run("older version", func(t *testing.T) { - store := makeMemoryStore() - store.SetBasis(sharedtest.NewDataSetBuilder().Build()) - - item1Version := 10 - item1, collection1 := makeItem("key", item1Version, false) - - updates := store.ApplyDelta(collection1) - assert.True(t, updates[kind]["key"]) - - _, collection1a := makeItem("key", item1Version-1, true) - - updates = store.ApplyDelta(collection1a) - assert.False(t, updates[kind]["key"]) + tests := []struct { + name string + initialVersion int + nextVersion int + }{ + {"newer version", 10, 11}, + {"older version", 10, 9}, + {"same version", 10, 10}, + } - result, err := store.Get(kind, "key") - require.NoError(t, err) - assert.Equal(t, item1, result) - }) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + store := makeMemoryStore() + store.SetBasis(sharedtest.NewDataSetBuilder().Build()) - t.Run("same version", func(t *testing.T) { - store := makeMemoryStore() - store.SetBasis(sharedtest.NewDataSetBuilder().Build()) + initialItem, initialCollection := makeItem("key", test.initialVersion, false) - item1Version := 10 - item1, collection1 := makeItem("key", item1Version, false) - updated := store.ApplyDelta(collection1) - assert.True(t, updated[kind]["key"]) + store.ApplyDelta(initialCollection) + result, err := store.Get(kind, "key") + require.NoError(t, err) + assert.Equal(t, initialItem, result) - _, collection1a := makeItem("key", item1Version, true) - updated = store.ApplyDelta(collection1a) - assert.False(t, updated[kind]["key"]) + updatedItem, updatedCollection := makeItem("key", test.nextVersion, true) - result, err := store.Get(kind, "key") - require.NoError(t, err) - assert.Equal(t, item1, result) - }) + store.ApplyDelta(updatedCollection) + result, err = store.Get(kind, "key") + require.NoError(t, err) + assert.Equal(t, updatedItem, result) + }) + } }) t.Run("deletes", func(t *testing.T) { - t.Run("newer version", func(t *testing.T) { - store := makeMemoryStore() - store.SetBasis(sharedtest.NewDataSetBuilder().Build()) - - item1, collection1 := makeItem("key", 10, false) - updated := store.ApplyDelta(collection1) - assert.True(t, updated[kind]["key"]) - - item1a, collection1a := deleteItem("key", item1.Version+1) - updated = store.ApplyDelta(collection1a) - assert.True(t, updated[kind]["key"]) - - result, err := store.Get(kind, "key") - require.NoError(t, err) - assert.Equal(t, item1a, result) - }) - - t.Run("older version", func(t *testing.T) { - store := makeMemoryStore() - store.SetBasis(sharedtest.NewDataSetBuilder().Build()) - - item1, collection1 := makeItem("key", 10, false) - updated := store.ApplyDelta(collection1) - assert.True(t, updated[kind]["key"]) - - _, collection1a := deleteItem("key", item1.Version-1) - updated = store.ApplyDelta(collection1a) - assert.False(t, updated[kind]["key"]) - - result, err := store.Get(kind, "key") - require.NoError(t, err) - assert.Equal(t, item1, result) - }) - - t.Run("same version", func(t *testing.T) { - store := makeMemoryStore() - store.SetBasis(sharedtest.NewDataSetBuilder().Build()) - - item1, collection1 := makeItem("key", 10, false) - updated := store.ApplyDelta(collection1) - assert.True(t, updated[kind]["key"]) - - _, collection1a := deleteItem("key", item1.Version) - updated = store.ApplyDelta(collection1a) - assert.False(t, updated[kind]["key"]) - - result, err := store.Get(kind, "key") - require.NoError(t, err) - assert.Equal(t, item1, result) - }) + tests := []struct { + name string + initialVersion int + nextVersion int + }{ + {"newer version", 10, 11}, + {"older version", 10, 9}, + {"same version", 10, 10}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + store := makeMemoryStore() + store.SetBasis(sharedtest.NewDataSetBuilder().Build()) + + initialItem, initialCollection := makeItem("key", test.initialVersion, false) + store.ApplyDelta(initialCollection) + result, err := store.Get(kind, "key") + require.NoError(t, err) + assert.Equal(t, initialItem, result) + + _, updatedCollection := deleteItem("key", test.nextVersion) + store.ApplyDelta(updatedCollection) + result, err = store.Get(kind, "key") + require.NoError(t, err) + require.IsType(t, ldstoretypes.ItemDescriptor{}.NotFound(), result) + }) + } }) }) }