From abba61723ae085252e60f399eeaf45c64b6683fb Mon Sep 17 00:00:00 2001 From: ionutboangiu Date: Fri, 7 Jun 2024 10:50:18 +0300 Subject: [PATCH] Optimize removeFilterIndexesForFilter func (#4357) Previously made two trips (get and set) for each group of items from an index key being removed. Now, we fetch indexes once at the beginning and store the updated indexes once at the end. There was a difference compared to the previous v0.10 implementation regarding the get/set behaviour. On v0.11, we used to get only the items separately for each index and update only that index. Even though it might have been more efficient, trips to the db are still too expensive so it didn't matter much in the end. --- engine/libindex.go | 38 +++++------ engine/libindex_test.go | 136 +++++++++++++++++++++++++++++++++------- 2 files changed, 133 insertions(+), 41 deletions(-) diff --git a/engine/libindex.go b/engine/libindex.go index a13bafc31..5b2d82f3f 100644 --- a/engine/libindex.go +++ b/engine/libindex.go @@ -783,32 +783,32 @@ func UpdateFilterIndex(dm *DataManager, oldFlt, newFlt *Filter) (err error) { return } -// removeFilterIndexesForFilter removes the itemID for the index keys -// used to remove the old indexes when a filter is updated +// removeFilterIndexesForFilter removes itemIDs from the specified filter index keys. +// Used to update the index map when a filter is modified. func removeFilterIndexesForFilter(dm *DataManager, idxItmType, tnt string, - removeIndexKeys []string, itemIDs utils.StringSet) (err error) { + removeIndexKeys []string, itemIDs utils.StringSet) error { refID := guardian.Guardian.GuardIDs(utils.EmptyString, config.CgrConfig().GeneralCfg().LockingTimeout, idxItmType+tnt) defer guardian.Guardian.UnguardIDs(refID) - for _, idxKey := range removeIndexKeys { // delete old filters indexes for this item - var remIndx map[string]utils.StringSet - if remIndx, err = dm.GetIndexes(idxItmType, tnt, - idxKey, true, false); err != nil { - if err != utils.ErrNotFound { - return - } - err = nil - continue - } - for idx := range itemIDs { - remIndx[idxKey].Remove(idx) - } - if err = dm.SetIndexes(idxItmType, tnt, remIndx, true, utils.NonTransactional); err != nil { - return + // Retrieve current filter indexes. + fltrIdx, err := dm.GetIndexes(idxItmType, tnt, utils.EmptyString, true, false) + if err != nil { + if err != utils.ErrNotFound { + return err + } + return nil + } + + // Remove itemIDs from the specified index keys. + for _, idxKey := range removeIndexKeys { + for itemID := range itemIDs { + fltrIdx[idxKey].Remove(itemID) } } - return + + // Store the updated indexes. + return dm.SetIndexes(idxItmType, tnt, fltrIdx, true, utils.NonTransactional) } // IsDynamicDPPath check dynamic path with ~*stats, ~*resources, ~*accounts, ~*libphonenumber, ~*asm to not be indexed diff --git a/engine/libindex_test.go b/engine/libindex_test.go index a6cc12b97..41f63e401 100644 --- a/engine/libindex_test.go +++ b/engine/libindex_test.go @@ -24,6 +24,7 @@ import ( "github.com/cgrates/cgrates/config" "github.com/cgrates/cgrates/utils" + "github.com/google/go-cmp/cmp" ) func TestLibIndexIsDynamicDPPath(t *testing.T) { @@ -45,39 +46,130 @@ func TestLibIndexIsDynamicDPPath(t *testing.T) { } } -func TestLibIndexRemoveFilterIndexesForFilterErrnotFound(t *testing.T) { +func TestLibIndexRemoveFilterIndexesForFilter(t *testing.T) { cfg := config.NewDefaultCGRConfig() dataDB := NewInternalDB(nil, nil, true, cfg.DataDbCfg().Items) dm := NewDataManager(dataDB, cfg.CacheCfg(), nil) tntCtx := "cgrates.org:*sessions" - test := struct { + + tests := []struct { name string - idx map[string]utils.StringSet - keys []string + idx map[string]utils.StringSet // initial indexes map + keys []string // that will be removed from the index itemIDs utils.StringSet + want map[string]utils.StringSet // indexes map after remove }{ - name: "testing for ErrNotFound", - idx: map[string]utils.StringSet{ - "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), - "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + { + name: "remove one filter index from all profiles", + idx: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + keys: []string{"*string:~*req.Account:1001"}, + itemIDs: utils.NewStringSet([]string{"AP1", "AP2"}), + want: map[string]utils.StringSet{ + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + }, + { + name: "remove one filter index from one profile", + idx: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + keys: []string{"*string:~*req.Account:1001"}, + itemIDs: utils.NewStringSet([]string{"AP1"}), + want: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + }, + { + name: "attempt remove non-existent filter index", + idx: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + keys: []string{"*string:~*req.Account:notfound"}, + itemIDs: utils.NewStringSet([]string{"AP1", "AP2"}), + want: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + }, + { + name: "remove all filter indexes from one profile", + idx: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + keys: []string{"*string:~*req.Account:1001", "*string:~*req.Account:1002"}, + itemIDs: utils.NewStringSet([]string{"AP1"}), + want: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP2"}), + }, + }, + { + name: "remove all filter indexes from all profiles", + idx: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2"}), + "*string:~*req.Account:1002": utils.NewStringSet([]string{"AP1", "AP2"}), + }, + keys: []string{"*string:~*req.Account:1001", "*string:~*req.Account:1002"}, + itemIDs: utils.NewStringSet([]string{"AP1", "AP2"}), + want: nil, + }, + { + name: "remove multiple filter indexes from mixed profiles", + idx: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2", "AP3"}), + "*string:~*req.Destination:1010": utils.NewStringSet([]string{"AP2", "AP3"}), + "*string:~*req.Destination:1011": utils.NewStringSet([]string{"AP1", "AP3", "AP4"}), + "*string:~*req.Destination:1012": utils.NewStringSet([]string{"AP2"}), + }, + keys: []string{"*string:~*req.Destination:1010", "*string:~*req.Destination:1012"}, + itemIDs: utils.NewStringSet([]string{"AP2", "AP3"}), + want: map[string]utils.StringSet{ + "*string:~*req.Account:1001": utils.NewStringSet([]string{"AP1", "AP2", "AP3"}), + "*string:~*req.Destination:1011": utils.NewStringSet([]string{"AP1", "AP3", "AP4"}), + }, }, - keys: []string{"*string:~*req.Account:1001"}, - itemIDs: utils.NewStringSet([]string{"AP1", "AP2"}), } - t.Run(test.name, func(t *testing.T) { - t.Cleanup(func() { - if err := dataDB.Flush(""); err != nil { - t.Logf("failed to flush dataDB: %v", err) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Cleanup(func() { + if err := dataDB.Flush(""); err != nil { + t.Logf("failed to flush dataDB: %v", err) + } + }) + if err := dm.SetIndexes(utils.CacheAttributeFilterIndexes, tntCtx, test.idx, true, ""); err != nil { + t.Fatalf("dm.SetFilterIndexes() returned unexpected error: %v", err) + } + if err := removeFilterIndexesForFilter(dm, utils.CacheAttributeFilterIndexes, + tntCtx, test.keys, test.itemIDs); err != nil { + t.Fatalf("removeFilterIndexesForFilter() returned unexpected error: %v", err) + } + got, err := dm.GetIndexes(utils.CacheAttributeFilterIndexes, tntCtx, "", true, false) + switch len(test.want) { + case 0: + if !errors.Is(err, utils.ErrNotFound) { + t.Fatalf("dm.GetFilterIndexes(%q,%q) err = %v, want %v", + utils.CacheAttributeFilterIndexes, tntCtx, err, utils.ErrNotFound) + } + default: + if err != nil { + t.Fatalf("dm.GetFilterIndexes(%q,%q) returned unexpected error: %v", + utils.CacheAttributeFilterIndexes, tntCtx, err) + } + } + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("dm.GetFilterIndexes(%q,%q) returned unexpected indexes (-want +got): \n%s", + utils.CacheAttributeFilterIndexes, tntCtx, diff) } }) - if err := dm.SetIndexes(utils.CacheAttributeFilterIndexes, tntCtx, test.idx, true, ""); err != nil { - t.Fatalf("dm.SetIndexes() returned unexpected error: %v", err) - } - err := removeFilterIndexesForFilter(dm, utils.CacheAttributeFilterIndexes, tntCtx, test.keys, test.itemIDs) - if err != nil && !errors.Is(err, utils.ErrNotFound) { - t.Fatalf("Expected error %v, got %v", utils.ErrNotFound, err) - } - }) + } } func TestLibIndexSplitFilterIndexErrWrongIdxKeyFormat(t *testing.T) {