[gold] Refactor and simplify the ignorestore implementation.
There's a nasty cyclical dependency between ignorestore and storage
which was papered over long ago. This removes the dependency, which
will make the refactoring effort in:
https://skia-review.googlesource.com/c/buildbot/+/219477
possible again.
Bug: skia:9096
Change-Id: Ieb96dfafde8f4a34d99a3fb0da9f9a158b6a4f52
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/219480
Reviewed-by: Ben Wagner aka dogben <benjaminwagner@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
diff --git a/golden/cmd/sampler/main.go b/golden/cmd/sampler/main.go
index d8d35f2..061894e 100644
--- a/golden/cmd/sampler/main.go
+++ b/golden/cmd/sampler/main.go
@@ -8,7 +8,6 @@
"net/url"
"os"
"reflect"
- "time"
"go.skia.org/infra/go/auth"
"go.skia.org/infra/go/common"
@@ -23,6 +22,7 @@
"go.skia.org/infra/go/util"
"go.skia.org/infra/golden/go/expstorage/fs_expstore"
"go.skia.org/infra/golden/go/ignore"
+ "go.skia.org/infra/golden/go/ignore/ds_ignorestore"
"go.skia.org/infra/golden/go/serialize"
"go.skia.org/infra/golden/go/storage"
"go.skia.org/infra/golden/go/types"
@@ -115,7 +115,7 @@
// Get the ignore rules.
var err error
- if sample.IgnoreRules, err = ignoreStore.List(false); err != nil {
+ if sample.IgnoreRules, err = ignoreStore.List(); err != nil {
sklog.Fatalf("Error retrieving ignore rules: %s", err)
}
@@ -219,7 +219,7 @@
EventBus: evt,
}
- storages.IgnoreStore, err = ignore.NewCloudIgnoreStore(ds.DS, expStore, storages.GetTileStreamNow(time.Minute*20, "sampler-ignore-store"))
+ storages.IgnoreStore, err = ds_ignorestore.New(ds.DS)
if err != nil {
sklog.Fatalf("Unable to create cloud ignorestore: %s", err)
}
diff --git a/golden/cmd/skiacorrectness/main.go b/golden/cmd/skiacorrectness/main.go
index d8d8c81..b21288b 100644
--- a/golden/cmd/skiacorrectness/main.go
+++ b/golden/cmd/skiacorrectness/main.go
@@ -47,6 +47,7 @@
"go.skia.org/infra/golden/go/diffstore/mapper/disk_mapper"
"go.skia.org/infra/golden/go/expstorage/fs_expstore"
"go.skia.org/infra/golden/go/ignore"
+ "go.skia.org/infra/golden/go/ignore/ds_ignorestore"
"go.skia.org/infra/golden/go/indexer"
"go.skia.org/infra/golden/go/search"
"go.skia.org/infra/golden/go/shared"
@@ -424,10 +425,7 @@
// openSite indicates whether this can expose all end-points. The user still has to be authenticated.
openSite := (*pubWhiteList == WHITELIST_ALL) || *forceLogin
- // TODO(kjlubick): storage.Storage contains an IgnoreStore, so we can't pass ignore
- // a storage.Storage directly. There probably shouldn't be one monolithic storage object,
- // but each package could define its own.
- if storages.IgnoreStore, err = ignore.NewCloudIgnoreStore(ds.DS, storages.ExpectationsStore, storages.GetTileStreamNow(time.Minute, "gold-ignore-store")); err != nil {
+ if storages.IgnoreStore, err = ds_ignorestore.New(ds.DS); err != nil {
sklog.Fatalf("Unable to create cloud ignorestore: %s", err)
}
diff --git a/golden/go/diffstore/bench_test.go b/golden/go/diffstore/bench_test.go
index 13fd6cd..c14feae 100644
--- a/golden/go/diffstore/bench_test.go
+++ b/golden/go/diffstore/bench_test.go
@@ -10,7 +10,7 @@
"go.skia.org/infra/golden/go/diff"
"go.skia.org/infra/golden/go/diffstore/mapper/disk_mapper"
d_utils "go.skia.org/infra/golden/go/diffstore/testutils"
- "go.skia.org/infra/golden/go/ignore"
+ "go.skia.org/infra/golden/go/ignore/mem_ignorestore"
"go.skia.org/infra/golden/go/mocks"
"go.skia.org/infra/golden/go/serialize"
"go.skia.org/infra/golden/go/types"
@@ -28,7 +28,7 @@
client := mocks.GetHTTPClient(b)
defer testutils.RemoveAll(b, baseDir)
- memIgnoreStore := ignore.NewMemIgnoreStore()
+ memIgnoreStore := mem_ignorestore.New()
for _, ir := range sample.IgnoreRules {
assert.NoError(b, memIgnoreStore.Create(ir))
}
diff --git a/golden/go/ignore/cloud_ignorestore.go b/golden/go/ignore/cloud_ignorestore.go
deleted file mode 100644
index 1e68bfe..0000000
--- a/golden/go/ignore/cloud_ignorestore.go
+++ /dev/null
@@ -1,249 +0,0 @@
-package ignore
-
-import (
- "context"
- "fmt"
- "sort"
- "sync/atomic"
-
- "cloud.google.com/go/datastore"
- "go.skia.org/infra/go/ds"
- "go.skia.org/infra/go/skerr"
- "go.skia.org/infra/go/sklog"
- "go.skia.org/infra/golden/go/dsutil"
- "go.skia.org/infra/golden/go/expstorage"
- "go.skia.org/infra/golden/go/types"
- "golang.org/x/sync/errgroup"
-)
-
-// cloudIgnoreStore implements the IgnoreStore interface
-type cloudIgnoreStore struct {
- client *datastore.Client
- recentKeysList *dsutil.RecentKeysList
- revision int64
- lastCpxTile types.ComplexTile
- expStore expstorage.ExpectationsStore
- cpxTileStream <-chan types.ComplexTile
-}
-
-// NewCloudIgnoreStore returns an IgnoreStore instance that is backed by Cloud Datastore.
-func NewCloudIgnoreStore(client *datastore.Client, expStore expstorage.ExpectationsStore, tileStream <-chan types.ComplexTile) (IgnoreStore, error) {
- if client == nil {
- return nil, skerr.Fmt("Received nil for datastore client.")
- }
-
- containerKey := ds.NewKey(ds.HELPER_RECENT_KEYS)
- containerKey.Name = "ignore:recent-keys"
-
- store := &cloudIgnoreStore{
- client: client,
- recentKeysList: dsutil.NewRecentKeysList(client, containerKey, dsutil.DefaultConsistencyDelta),
- expStore: expStore,
- cpxTileStream: tileStream,
- }
- return store, nil
-}
-
-// Create implements the IgnoreStore interface.
-func (c *cloudIgnoreStore) Create(ignoreRule *IgnoreRule) error {
- createFn := func(tx *datastore.Transaction) error {
- key := dsutil.TimeSortableKey(ds.IGNORE_RULE, 0)
- ignoreRule.ID = key.ID
-
- // Add the new rule and put its key with the recently added keys.
- if _, err := tx.Put(key, ignoreRule); err != nil {
- return err
- }
-
- return c.recentKeysList.Add(tx, key)
- }
-
- // Run the relevant updates in a transaction.
- _, err := c.client.RunInTransaction(context.TODO(), createFn)
-
- // TODO(stephana): Look into removing the revision feature. I don't think
- // this is really necessary going forward.
-
- if err == nil {
- atomic.AddInt64(&c.revision, 1)
- }
- return err
-}
-
-// List implements the IgnoreStore interface.
-func (c *cloudIgnoreStore) List(addCounts bool) ([]*IgnoreRule, error) {
- ctx := context.TODO()
- var egroup errgroup.Group
- var queriedKeys []*datastore.Key
- egroup.Go(func() error {
- // Query all entities.
- query := ds.NewQuery(ds.IGNORE_RULE).KeysOnly()
- var err error
- queriedKeys, err = c.client.GetAll(ctx, query, nil)
- return err
- })
-
- var recently *dsutil.Recently
- egroup.Go(func() error {
- var err error
- recently, err = c.recentKeysList.GetRecent()
- return err
- })
-
- if err := egroup.Wait(); err != nil {
- return nil, skerr.Fmt("Error getting keys of ignore rules: %s", err)
- }
-
- // Merge the keys to get all of the current keys.
- allKeys := recently.Combine(queriedKeys)
- if len(allKeys) == 0 {
- return []*IgnoreRule{}, nil
- }
-
- ret := make([]*IgnoreRule, len(allKeys))
- if err := c.client.GetMulti(ctx, allKeys, ret); err != nil {
- return nil, err
- }
- sort.Slice(ret, func(i, j int) bool { return ret[i].Expires.Before(ret[j].Expires) })
-
- if addCounts {
- var err error
- c.lastCpxTile, err = addIgnoreCounts(ret, c, c.lastCpxTile, c.expStore, c.cpxTileStream)
- if err != nil {
- sklog.Errorf("Unable to add counts to ignore list result: %s", err)
- }
- }
-
- return ret, nil
-}
-
-// Update implements the IgnoreStore interface.
-func (c *cloudIgnoreStore) Update(id int64, rule *IgnoreRule) error {
- ctx := context.TODO()
- key := ds.NewKey(ds.IGNORE_RULE)
- key.ID = id
- _, err := c.client.Mutate(ctx, datastore.NewUpdate(key, rule))
- if err == nil {
- atomic.AddInt64(&c.revision, 1)
- }
- return err
-}
-
-// Delete implements the IgnoreStore interface.
-func (c *cloudIgnoreStore) Delete(id int64) (int, error) {
- if id <= 0 {
- return 0, skerr.Fmt("Given id does not exist: %d", id)
- }
-
- deleteFn := func(tx *datastore.Transaction) error {
- key := ds.NewKey(ds.IGNORE_RULE)
- key.ID = id
-
- ignoreRule := &IgnoreRule{}
- if err := tx.Get(key, ignoreRule); err != nil {
- return err
- }
-
- if err := tx.Delete(key); err != nil {
- return err
- }
-
- return c.recentKeysList.Delete(tx, key)
- }
-
- // Run the relevant updates in a transaction.
- _, err := c.client.RunInTransaction(context.TODO(), deleteFn)
- if err != nil {
- // Don't report an error if the item did not exist.
- if err == datastore.ErrNoSuchEntity {
- sklog.Warningf("Could not delete ignore with id %d because it did not exist", id)
- return 0, nil
- }
- return 0, err
- }
-
- atomic.AddInt64(&c.revision, 1)
- return 1, nil
-}
-
-// Revision implements the IgnoreStore interface.
-func (c *cloudIgnoreStore) Revision() int64 {
- return atomic.LoadInt64(&c.revision)
-}
-
-// BuildRuleMatcher implements the IgnoreStore interface.
-func (c *cloudIgnoreStore) BuildRuleMatcher() (RuleMatcher, error) {
- return buildRuleMatcher(c)
-}
-
-// TODO(kjlubick): Add unit tests to addIgnoreCounts using mocks
-
-// addIgnoreCounts adds counts for the current tile to the given list of rules.
-func addIgnoreCounts(rules []*IgnoreRule, ignoreStore IgnoreStore, lastCpxTile types.ComplexTile, expStore expstorage.ExpectationsStore, tileStream <-chan types.ComplexTile) (types.ComplexTile, error) {
- if (expStore == nil) || (tileStream == nil) {
- return nil, fmt.Errorf("Either expStore or tileStream is nil. Cannot count ignores.")
- }
-
- exp, err := expStore.Get()
- if err != nil {
- return nil, err
- }
-
- ignoreMatcher, err := ignoreStore.BuildRuleMatcher()
- if err != nil {
- return nil, err
- }
-
- // Get the next tile.
- var cpxTile types.ComplexTile = nil
- select {
- case cpxTile = <-tileStream:
- default:
- cpxTile = lastCpxTile
- }
- if cpxTile == nil {
- return nil, fmt.Errorf("No tile available to count ignores")
- }
-
- // Count the untriaged digests in HEAD.
- // matchingDigests[rule.ID]map["testname:digest"]bool
- matchingDigests := make(map[int64]map[string]bool, len(rules))
- rulesByDigest := map[string]map[int64]bool{}
- tileWithIgnores := cpxTile.GetTile(types.IncludeIgnoredTraces)
- for _, trace := range tileWithIgnores.Traces {
- gTrace := trace.(*types.GoldenTrace)
- if matchRules, ok := ignoreMatcher(gTrace.Keys); ok {
- testName := gTrace.TestName()
- if digest := gTrace.LastDigest(); digest != types.MISSING_DIGEST && (exp.Classification(testName, digest) == types.UNTRIAGED) {
- k := string(testName) + ":" + string(digest)
- for _, r := range matchRules {
- // Add the digest to all matching rules.
- if t, ok := matchingDigests[r.ID]; ok {
- t[k] = true
- } else {
- matchingDigests[r.ID] = map[string]bool{k: true}
- }
-
- // Add the rule to the test-digest.
- if t, ok := rulesByDigest[k]; ok {
- t[r.ID] = true
- } else {
- rulesByDigest[k] = map[int64]bool{r.ID: true}
- }
- }
- }
- }
- }
-
- for _, r := range rules {
- r.Count = len(matchingDigests[r.ID])
- r.ExclusiveCount = 0
- for testDigestKey := range matchingDigests[r.ID] {
- // If exactly this one rule matches then account for it.
- if len(rulesByDigest[testDigestKey]) == 1 {
- r.ExclusiveCount++
- }
- }
- }
- return cpxTile, nil
-}
diff --git a/golden/go/ignore/common.go b/golden/go/ignore/common.go
index e816468..2611adc 100644
--- a/golden/go/ignore/common.go
+++ b/golden/go/ignore/common.go
@@ -2,8 +2,8 @@
import "net/url"
-func buildRuleMatcher(store IgnoreStore) (RuleMatcher, error) {
- rulesList, err := store.List(false)
+func BuildRuleMatcher(store IgnoreStore) (RuleMatcher, error) {
+ rulesList, err := store.List()
if err != nil {
return noopRuleMatcher, err
}
diff --git a/golden/go/ignore/ds_ignorestore/ds_ignorestore.go b/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
new file mode 100644
index 0000000..038209b
--- /dev/null
+++ b/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
@@ -0,0 +1,162 @@
+package ds_ignorestore
+
+import (
+ "context"
+ "sort"
+ "sync/atomic"
+
+ "cloud.google.com/go/datastore"
+ "go.skia.org/infra/go/ds"
+ "go.skia.org/infra/go/skerr"
+ "go.skia.org/infra/go/sklog"
+ "go.skia.org/infra/golden/go/dsutil"
+ "go.skia.org/infra/golden/go/ignore"
+ "golang.org/x/sync/errgroup"
+)
+
+// DSIgnoreStore implements the IgnoreStore interface
+type DSIgnoreStore struct {
+ client *datastore.Client
+ recentKeysList *dsutil.RecentKeysList
+ revision int64
+}
+
+// New returns an IgnoreStore instance that is backed by Cloud Datastore.
+func New(client *datastore.Client) (*DSIgnoreStore, error) {
+ if client == nil {
+ return nil, skerr.Fmt("Received nil for datastore client.")
+ }
+
+ containerKey := ds.NewKey(ds.HELPER_RECENT_KEYS)
+ containerKey.Name = "ignore:recent-keys"
+
+ store := &DSIgnoreStore{
+ client: client,
+ recentKeysList: dsutil.NewRecentKeysList(client, containerKey, dsutil.DefaultConsistencyDelta),
+ }
+ return store, nil
+}
+
+// Create implements the IgnoreStore interface.
+func (c *DSIgnoreStore) Create(ignoreRule *ignore.IgnoreRule) error {
+ createFn := func(tx *datastore.Transaction) error {
+ key := dsutil.TimeSortableKey(ds.IGNORE_RULE, 0)
+ ignoreRule.ID = key.ID
+
+ // Add the new rule and put its key with the recently added keys.
+ if _, err := tx.Put(key, ignoreRule); err != nil {
+ return err
+ }
+
+ return c.recentKeysList.Add(tx, key)
+ }
+
+ // Run the relevant updates in a transaction.
+ _, err := c.client.RunInTransaction(context.TODO(), createFn)
+
+ // TODO(stephana): Look into removing the revision feature. I don't think
+ // this is really necessary going forward.
+
+ if err == nil {
+ atomic.AddInt64(&c.revision, 1)
+ }
+ return err
+}
+
+// List implements the IgnoreStore interface.
+func (c *DSIgnoreStore) List() ([]*ignore.IgnoreRule, error) {
+ ctx := context.TODO()
+ var egroup errgroup.Group
+ var queriedKeys []*datastore.Key
+ egroup.Go(func() error {
+ // Query all entities.
+ query := ds.NewQuery(ds.IGNORE_RULE).KeysOnly()
+ var err error
+ queriedKeys, err = c.client.GetAll(ctx, query, nil)
+ return err
+ })
+
+ var recently *dsutil.Recently
+ egroup.Go(func() error {
+ var err error
+ recently, err = c.recentKeysList.GetRecent()
+ return err
+ })
+
+ if err := egroup.Wait(); err != nil {
+ return nil, skerr.Fmt("Error getting keys of ignore rules: %s", err)
+ }
+
+ // Merge the keys to get all of the current keys.
+ allKeys := recently.Combine(queriedKeys)
+ if len(allKeys) == 0 {
+ return []*ignore.IgnoreRule{}, nil
+ }
+
+ ret := make([]*ignore.IgnoreRule, len(allKeys))
+ if err := c.client.GetMulti(ctx, allKeys, ret); err != nil {
+ return nil, err
+ }
+ sort.Slice(ret, func(i, j int) bool { return ret[i].Expires.Before(ret[j].Expires) })
+
+ return ret, nil
+}
+
+// Update implements the IgnoreStore interface.
+func (c *DSIgnoreStore) Update(id int64, rule *ignore.IgnoreRule) error {
+ ctx := context.TODO()
+ key := ds.NewKey(ds.IGNORE_RULE)
+ key.ID = id
+ _, err := c.client.Mutate(ctx, datastore.NewUpdate(key, rule))
+ if err == nil {
+ atomic.AddInt64(&c.revision, 1)
+ }
+ return err
+}
+
+// Delete implements the IgnoreStore interface.
+func (c *DSIgnoreStore) Delete(id int64) (int, error) {
+ if id <= 0 {
+ return 0, skerr.Fmt("Given id does not exist: %d", id)
+ }
+
+ deleteFn := func(tx *datastore.Transaction) error {
+ key := ds.NewKey(ds.IGNORE_RULE)
+ key.ID = id
+
+ ignoreRule := &ignore.IgnoreRule{}
+ if err := tx.Get(key, ignoreRule); err != nil {
+ return err
+ }
+
+ if err := tx.Delete(key); err != nil {
+ return err
+ }
+
+ return c.recentKeysList.Delete(tx, key)
+ }
+
+ // Run the relevant updates in a transaction.
+ _, err := c.client.RunInTransaction(context.TODO(), deleteFn)
+ if err != nil {
+ // Don't report an error if the item did not exist.
+ if err == datastore.ErrNoSuchEntity {
+ sklog.Warningf("Could not delete ignore with id %d because it did not exist", id)
+ return 0, nil
+ }
+ return 0, err
+ }
+
+ atomic.AddInt64(&c.revision, 1)
+ return 1, nil
+}
+
+// Revision implements the IgnoreStore interface.
+func (c *DSIgnoreStore) Revision() int64 {
+ return atomic.LoadInt64(&c.revision)
+}
+
+// BuildRuleMatcher implements the IgnoreStore interface.
+func (c *DSIgnoreStore) BuildRuleMatcher() (ignore.RuleMatcher, error) {
+ return ignore.BuildRuleMatcher(c)
+}
diff --git a/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go b/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
new file mode 100644
index 0000000..0e1e14e
--- /dev/null
+++ b/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
@@ -0,0 +1,23 @@
+package ds_ignorestore
+
+import (
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "go.skia.org/infra/go/ds"
+ ds_testutil "go.skia.org/infra/go/ds/testutil"
+ "go.skia.org/infra/go/testutils/unittest"
+ "go.skia.org/infra/golden/go/ignore/testutils"
+)
+
+func TestCloudIgnoreStore(t *testing.T) {
+ unittest.LargeTest(t)
+
+ // Run to the locally running emulator.
+ cleanup := ds_testutil.InitDatastore(t, ds.IGNORE_RULE, ds.HELPER_RECENT_KEYS)
+ defer cleanup()
+
+ store, err := New(ds.DS)
+ assert.NoError(t, err)
+ testutils.IgnoreStoreAll(t, store)
+}
diff --git a/golden/go/ignore/ignorestore.go b/golden/go/ignore/ignorestore.go
index 89cff4f..82c63d9 100644
--- a/golden/go/ignore/ignorestore.go
+++ b/golden/go/ignore/ignorestore.go
@@ -3,7 +3,6 @@
import (
"fmt"
"net/url"
- "sync"
"time"
)
@@ -19,12 +18,7 @@
Create(*IgnoreRule) error
// List returns all ignore rules in the ignore store.
- // 'addCounts' indicates whether to include counts how often an ignore
- // rule appears in the current tile.
- // TODO(kjlubick): Remove the addCounts flag in the signature of the List function
- // and expose the AddIgnoreCounts function. This would remove the expsStore and
- // tileStream members of the cloudIgnoreStore struct and simplify the interface.
- List(addCounts bool) ([]*IgnoreRule, error)
+ List() ([]*IgnoreRule, error)
// Updates an IgnoreRule.
Update(id int64, rule *IgnoreRule) error
@@ -46,14 +40,12 @@
// IgnoreRule is the GUI struct for dealing with Ignore rules.
type IgnoreRule struct {
- ID int64 `json:"id,string"`
- Name string `json:"name"`
- UpdatedBy string `json:"updatedBy"`
- Expires time.Time `json:"expires"`
- Query string `json:"query"`
- Note string `json:"note"`
- Count int `json:"count" datastore:"-"`
- ExclusiveCount int `json:"exclusiveCount" datastore:"-"`
+ ID int64 `json:"id,string"`
+ Name string `json:"name"`
+ UpdatedBy string `json:"updatedBy"`
+ Expires time.Time `json:"expires"`
+ Query string `json:"query"`
+ Note string `json:"note"`
}
// ToQuery makes a slice of url.Values from the given slice of IngoreRules.
@@ -79,102 +71,6 @@
}
}
-// MemIgnoreStore is an in-memory implementation of IgnoreStore.
-type MemIgnoreStore struct {
- rules []*IgnoreRule
- mutex sync.Mutex
- nextId int64
- revision int64
-}
-
-func NewMemIgnoreStore() IgnoreStore {
- return &MemIgnoreStore{
- rules: []*IgnoreRule{},
- }
-}
-
-func (m *MemIgnoreStore) inc() {
- m.revision += 1
-}
-
-// Create, see IgnoreStore interface.
-func (m *MemIgnoreStore) Create(rule *IgnoreRule) error {
- m.mutex.Lock()
- defer m.mutex.Unlock()
-
- rule.ID = m.nextId
- m.nextId++
- m.rules = append(m.rules, rule)
- m.inc()
- return nil
-}
-
-// List, see IgnoreStore interface.
-func (m *MemIgnoreStore) List(addCounts bool) ([]*IgnoreRule, error) {
- m.mutex.Lock()
- defer m.mutex.Unlock()
-
- m.expire()
- result := make([]*IgnoreRule, len(m.rules))
- copy(result, m.rules)
- return result, nil
-}
-
-// Update, see IgnoreStore interface.
-func (m *MemIgnoreStore) Update(id int64, updated *IgnoreRule) error {
- m.mutex.Lock()
- defer m.mutex.Unlock()
-
- for i := range m.rules {
- if updated.ID == id {
- m.rules[i] = updated
- m.inc()
- return nil
- }
- }
-
- return fmt.Errorf("Did not find an IgnoreRule with id: %d", id)
-}
-
-// Delete, see IgnoreStore interface.
-func (m *MemIgnoreStore) Delete(id int64) (int, error) {
- m.mutex.Lock()
- defer m.mutex.Unlock()
-
- for idx, rule := range m.rules {
- if rule.ID == id {
- m.rules = append(m.rules[:idx], m.rules[idx+1:]...)
- m.inc()
- return 1, nil
- }
- }
-
- return 0, nil
-}
-
-func (m *MemIgnoreStore) Revision() int64 {
- m.mutex.Lock()
- defer m.mutex.Unlock()
-
- return m.revision
-}
-
-func (m *MemIgnoreStore) expire() {
- newrules := make([]*IgnoreRule, 0, len(m.rules))
- now := time.Now()
- for _, rule := range m.rules {
- if rule.Expires.After(now) {
- newrules = append(newrules, rule)
- }
- }
- m.rules = newrules
-}
-
-// BuildRuleMatcher, see IgnoreStore interface.
-func (m *MemIgnoreStore) BuildRuleMatcher() (RuleMatcher, error) {
- return buildRuleMatcher(m)
-}
-
// TODO(stephana): Factor out QueryRule into the shared library and consolidate
// with the Matches function in the ./go/tiling package.
diff --git a/golden/go/ignore/ignorestore_test.go b/golden/go/ignore/ignorestore_test.go
index eee52c3..6b6117f 100644
--- a/golden/go/ignore/ignorestore_test.go
+++ b/golden/go/ignore/ignorestore_test.go
@@ -6,138 +6,9 @@
"time"
assert "github.com/stretchr/testify/require"
- "go.skia.org/infra/go/ds"
- ds_testutil "go.skia.org/infra/go/ds/testutil"
"go.skia.org/infra/go/testutils/unittest"
)
-func TestTestMemIgnoreStore(t *testing.T) {
- unittest.SmallTest(t)
- memStore := NewMemIgnoreStore()
- testIgnoreStore(t, memStore)
-}
-
-func TestCloudIgnoreStore(t *testing.T) {
- unittest.LargeTest(t)
-
- // Run to the locally running emulator.
- cleanup := ds_testutil.InitDatastore(t,
- ds.IGNORE_RULE,
- ds.HELPER_RECENT_KEYS)
- defer cleanup()
-
- store, err := NewCloudIgnoreStore(ds.DS, nil, nil)
- assert.NoError(t, err)
- testIgnoreStore(t, store)
-}
-
-func testIgnoreStore(t *testing.T, store IgnoreStore) {
- // Add a few instances.
- r1 := NewIgnoreRule("jon@example.com", time.Now().Add(time.Hour), "config=gpu", "reason")
- r2 := NewIgnoreRule("jim@example.com", time.Now().Add(time.Minute*10), "config=8888", "No good reason.")
- r3 := NewIgnoreRule("jon@example.com", time.Now().Add(time.Minute*50), "extra=123&extra=abc", "Ignore multiple.")
- r4 := NewIgnoreRule("jon@example.com", time.Now().Add(time.Minute*100), "extra=123&extra=abc&config=8888", "Ignore multiple.")
- assert.Equal(t, int64(0), store.Revision())
- assert.NoError(t, store.Create(r1))
- assert.NoError(t, store.Create(r2))
- assert.NoError(t, store.Create(r3))
- assert.NoError(t, store.Create(r4))
- assert.Equal(t, int64(4), store.Revision())
-
- allRules, err := store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 4, len(allRules))
- assert.Equal(t, int64(4), store.Revision())
-
- // Test the rule matcher
- matcher, err := store.BuildRuleMatcher()
- assert.NoError(t, err)
- found, ok := matcher(map[string]string{"config": "565"})
- assert.False(t, ok)
- assert.Equal(t, []*IgnoreRule{}, found)
- found, ok = matcher(map[string]string{"config": "8888"})
- assert.True(t, ok)
- assert.Equal(t, 1, len(found))
- found, ok = matcher(map[string]string{"extra": "123"})
- assert.True(t, ok)
- assert.Equal(t, 1, len(found))
- found, ok = matcher(map[string]string{"extra": "abc"})
- assert.True(t, ok)
- assert.Equal(t, 1, len(found))
- found, ok = matcher(map[string]string{"extra": "abc", "config": "8888"})
- assert.True(t, ok)
- assert.Equal(t, 3, len(found))
- found, ok = matcher(map[string]string{"extra": "abc", "config": "gpu"})
- assert.True(t, ok)
- assert.Equal(t, 2, len(found))
- assert.Equal(t, int64(4), store.Revision())
-
- // Remove the third and fourth rule
- delCount, err := store.Delete(r3.ID)
- assert.NoError(t, err)
- assert.Equal(t, 1, delCount)
- allRules, err = store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 3, len(allRules))
-
- delCount, err = store.Delete(r4.ID)
- assert.NoError(t, err)
- assert.Equal(t, 1, delCount)
- allRules, err = store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 2, len(allRules))
- assert.Equal(t, int64(6), store.Revision())
-
- for _, oneRule := range allRules {
- assert.True(t, (oneRule.ID == r1.ID) || (oneRule.ID == r2.ID))
- }
-
- delCount, err = store.Delete(r1.ID)
- assert.NoError(t, err)
- assert.Equal(t, 1, delCount)
- allRules, err = store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 1, len(allRules))
- assert.Equal(t, r2.ID, allRules[0].ID)
- assert.Equal(t, int64(7), store.Revision())
-
- // Update a rule.
- updatedRule := *allRules[0]
- updatedRule.Note = "an updated rule"
- err = store.Update(updatedRule.ID, &updatedRule)
- assert.NoError(t, err, "Update should succeed.")
- allRules, err = store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 1, len(allRules))
- assert.Equal(t, r2.ID, allRules[0].ID)
- assert.Equal(t, "an updated rule", allRules[0].Note)
- assert.Equal(t, int64(8), store.Revision())
-
- // Try to update a non-existent rule.
- updatedRule = *allRules[0]
- err = store.Update(100001, &updatedRule)
- assert.Error(t, err, "Update should fail for a bad id.")
- assert.Equal(t, int64(8), store.Revision())
-
- delCount, err = store.Delete(r2.ID)
- assert.NoError(t, err)
- assert.Equal(t, 1, delCount)
-
- allRules, err = store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 0, len(allRules))
- assert.Equal(t, int64(9), store.Revision())
-
- // This id doesn't exist, so we shouldn't be able to delete it.
- delCount, err = store.Delete(1000000)
- assert.NoError(t, err)
- assert.Equal(t, delCount, 0)
- allRules, err = store.List(false)
- assert.NoError(t, err)
- assert.Equal(t, 0, len(allRules))
- assert.Equal(t, int64(9), store.Revision())
-}
-
func TestToQuery(t *testing.T) {
unittest.SmallTest(t)
queries, err := ToQuery([]*IgnoreRule{})
diff --git a/golden/go/ignore/mem_ignorestore/mem_ignorestore.go b/golden/go/ignore/mem_ignorestore/mem_ignorestore.go
new file mode 100644
index 0000000..0cb2ef7
--- /dev/null
+++ b/golden/go/ignore/mem_ignorestore/mem_ignorestore.go
@@ -0,0 +1,105 @@
+package mem_ignorestore
+
+import (
+ "fmt"
+ "sync"
+ "time"
+
+ "go.skia.org/infra/golden/go/ignore"
+)
+
+// MemIgnoreStore is an in-memory implementation of IgnoreStore.
+type MemIgnoreStore struct {
+ rules []*ignore.IgnoreRule
+ mutex sync.Mutex
+ nextId int64
+ revision int64
+}
+
+func New() *MemIgnoreStore {
+ return &MemIgnoreStore{
+ rules: []*ignore.IgnoreRule{},
+ }
+}
+
+func (m *MemIgnoreStore) inc() {
+ m.revision += 1
+}
+
+// Create, see IgnoreStore interface.
+func (m *MemIgnoreStore) Create(rule *ignore.IgnoreRule) error {
+ m.mutex.Lock()
+ defer m.mutex.Unlock()
+
+ rule.ID = m.nextId
+ m.nextId++
+ m.rules = append(m.rules, rule)
+ m.inc()
+ return nil
+}
+
+// List, see IgnoreStore interface.
+func (m *MemIgnoreStore) List() ([]*ignore.IgnoreRule, error) {
+ m.mutex.Lock()
+ defer m.mutex.Unlock()
+
+ m.expire()
+ result := make([]*ignore.IgnoreRule, len(m.rules))
+ copy(result, m.rules)
+ return result, nil
+}
+
+// Update, see IgnoreStore interface.
+func (m *MemIgnoreStore) Update(id int64, updated *ignore.IgnoreRule) error {
+ m.mutex.Lock()
+ defer m.mutex.Unlock()
+
+ for i := range m.rules {
+ if updated.ID == id {
+ m.rules[i] = updated
+ m.inc()
+ return nil
+ }
+ }
+
+ return fmt.Errorf("Did not find an IgnoreRule with id: %d", id)
+}
+
+// Delete, see IgnoreStore interface.
+func (m *MemIgnoreStore) Delete(id int64) (int, error) {
+ m.mutex.Lock()
+ defer m.mutex.Unlock()
+
+ for idx, rule := range m.rules {
+ if rule.ID == id {
+ m.rules = append(m.rules[:idx], m.rules[idx+1:]...)
+ m.inc()
+ return 1, nil
+ }
+ }
+
+ return 0, nil
+}
+
+func (m *MemIgnoreStore) Revision() int64 {
+ m.mutex.Lock()
+ defer m.mutex.Unlock()
+
+ return m.revision
+}
+
+func (m *MemIgnoreStore) expire() {
+ newrules := make([]*ignore.IgnoreRule, 0, len(m.rules))
+ now := time.Now()
+ for _, rule := range m.rules {
+ if rule.Expires.After(now) {
+ newrules = append(newrules, rule)
+ }
+ }
+ m.rules = newrules
+}
+
+// BuildRuleMatcher, see IgnoreStore interface.
+func (m *MemIgnoreStore) BuildRuleMatcher() (ignore.RuleMatcher, error) {
+ return ignore.BuildRuleMatcher(m)
+}
diff --git a/golden/go/ignore/mem_ignorestore/mem_ignorestore_test.go b/golden/go/ignore/mem_ignorestore/mem_ignorestore_test.go
new file mode 100644
index 0000000..06a7fb8
--- /dev/null
+++ b/golden/go/ignore/mem_ignorestore/mem_ignorestore_test.go
@@ -0,0 +1,14 @@
+package mem_ignorestore
+
+import (
+ "testing"
+
+ "go.skia.org/infra/go/testutils/unittest"
+ "go.skia.org/infra/golden/go/ignore/testutils"
+)
+
+func TestTestMemIgnoreStore(t *testing.T) {
+ unittest.SmallTest(t)
+ memStore := New()
+ testutils.IgnoreStoreAll(t, memStore)
+}
diff --git a/golden/go/ignore/monitor.go b/golden/go/ignore/monitor.go
index a7eb48a..89dd749 100644
--- a/golden/go/ignore/monitor.go
+++ b/golden/go/ignore/monitor.go
@@ -9,7 +9,7 @@
)
func oneStep(store IgnoreStore, metric metrics2.Int64Metric) error {
- list, err := store.List(false)
+ list, err := store.List()
if err != nil {
return err
}
diff --git a/golden/go/ignore/testutils/common.go b/golden/go/ignore/testutils/common.go
new file mode 100644
index 0000000..cf0e25c
--- /dev/null
+++ b/golden/go/ignore/testutils/common.go
@@ -0,0 +1,116 @@
+package testutils
+
+import (
+ "time"
+
+ assert "github.com/stretchr/testify/require"
+ "go.skia.org/infra/go/sktest"
+ "go.skia.org/infra/golden/go/ignore"
+)
+
+func IgnoreStoreAll(t sktest.TestingT, store ignore.IgnoreStore) {
+ // Add a few instances.
+ r1 := ignore.NewIgnoreRule("jon@example.com", time.Now().Add(time.Hour), "config=gpu", "reason")
+ r2 := ignore.NewIgnoreRule("jim@example.com", time.Now().Add(time.Minute*10), "config=8888", "No good reason.")
+ r3 := ignore.NewIgnoreRule("jon@example.com", time.Now().Add(time.Minute*50), "extra=123&extra=abc", "Ignore multiple.")
+ r4 := ignore.NewIgnoreRule("jon@example.com", time.Now().Add(time.Minute*100), "extra=123&extra=abc&config=8888", "Ignore multiple.")
+ assert.Equal(t, int64(0), store.Revision())
+ assert.NoError(t, store.Create(r1))
+ assert.NoError(t, store.Create(r2))
+ assert.NoError(t, store.Create(r3))
+ assert.NoError(t, store.Create(r4))
+ assert.Equal(t, int64(4), store.Revision())
+
+ allRules, err := store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 4, len(allRules))
+ assert.Equal(t, int64(4), store.Revision())
+
+ // Test the rule matcher
+ matcher, err := store.BuildRuleMatcher()
+ assert.NoError(t, err)
+ found, ok := matcher(map[string]string{"config": "565"})
+ assert.False(t, ok)
+ assert.Equal(t, []*ignore.IgnoreRule{}, found)
+ found, ok = matcher(map[string]string{"config": "8888"})
+ assert.True(t, ok)
+ assert.Equal(t, 1, len(found))
+ found, ok = matcher(map[string]string{"extra": "123"})
+ assert.True(t, ok)
+ assert.Equal(t, 1, len(found))
+ found, ok = matcher(map[string]string{"extra": "abc"})
+ assert.True(t, ok)
+ assert.Equal(t, 1, len(found))
+ found, ok = matcher(map[string]string{"extra": "abc", "config": "8888"})
+ assert.True(t, ok)
+ assert.Equal(t, 3, len(found))
+ found, ok = matcher(map[string]string{"extra": "abc", "config": "gpu"})
+ assert.True(t, ok)
+ assert.Equal(t, 2, len(found))
+ assert.Equal(t, int64(4), store.Revision())
+
+ // Remove the third and fourth rule
+ delCount, err := store.Delete(r3.ID)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, delCount)
+ allRules, err = store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 3, len(allRules))
+
+ delCount, err = store.Delete(r4.ID)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, delCount)
+ allRules, err = store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 2, len(allRules))
+ assert.Equal(t, int64(6), store.Revision())
+
+ for _, oneRule := range allRules {
+ assert.True(t, (oneRule.ID == r1.ID) || (oneRule.ID == r2.ID))
+ }
+
+ delCount, err = store.Delete(r1.ID)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, delCount)
+ allRules, err = store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 1, len(allRules))
+ assert.Equal(t, r2.ID, allRules[0].ID)
+ assert.Equal(t, int64(7), store.Revision())
+
+ // Update a rule.
+ updatedRule := *allRules[0]
+ updatedRule.Note = "an updated rule"
+ err = store.Update(updatedRule.ID, &updatedRule)
+ assert.NoError(t, err, "Update should succeed.")
+ allRules, err = store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 1, len(allRules))
+ assert.Equal(t, r2.ID, allRules[0].ID)
+ assert.Equal(t, "an updated rule", allRules[0].Note)
+ assert.Equal(t, int64(8), store.Revision())
+
+ // Try to update a non-existent rule.
+ updatedRule = *allRules[0]
+ err = store.Update(100001, &updatedRule)
+ assert.Error(t, err, "Update should fail for a bad id.")
+ assert.Equal(t, int64(8), store.Revision())
+
+ delCount, err = store.Delete(r2.ID)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, delCount)
+
+ allRules, err = store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 0, len(allRules))
+ assert.Equal(t, int64(9), store.Revision())
+
+ // This id doesn't exist, so we shouldn't be able to delete it.
+ delCount, err = store.Delete(1000000)
+ assert.NoError(t, err)
+ assert.Equal(t, delCount, 0)
+ allRules, err = store.List()
+ assert.NoError(t, err)
+ assert.Equal(t, 0, len(allRules))
+ assert.Equal(t, int64(9), store.Revision())
+}
diff --git a/golden/go/serialize/serialize.go b/golden/go/serialize/serialize.go
index c28eeb6..80258ad 100644
--- a/golden/go/serialize/serialize.go
+++ b/golden/go/serialize/serialize.go
@@ -61,14 +61,16 @@
// problems like https://crbug.com/skia/9070
// TODO(kjlubick): regenerate/replace test data and remove this struct
type legacyIgnoreRule struct {
- ID int64 `json:"id"`
- Name string `json:"name"`
- UpdatedBy string `json:"updatedBy"`
- Expires time.Time `json:"expires"`
- Query string `json:"query"`
- Note string `json:"note"`
- Count int `json:"count" datastore:"-"`
- ExclusiveCount int `json:"exclusiveCount" datastore:"-"`
+ ID int64 `json:"id"`
+ Name string `json:"name"`
+ UpdatedBy string `json:"updatedBy"`
+ Expires time.Time `json:"expires"`
+ Query string `json:"query"`
+ Note string `json:"note"`
+ // Count and ExclusiveCount were removed to simplify the ignorestore (and remove
+ // a noxious cyclical dependency.)
+ Count int `json:"count" datastore:"-"`
+ ExclusiveCount int `json:"exclusiveCount" datastore:"-"`
}
// DeserializeSample returns a new instance of Sample from the given reader. It
@@ -116,14 +118,12 @@
ret.IgnoreRules = make([]*ignore.IgnoreRule, 0, len(legacyRules))
for _, ir := range legacyRules {
ret.IgnoreRules = append(ret.IgnoreRules, &ignore.IgnoreRule{
- ID: ir.ID,
- Name: ir.Name,
- UpdatedBy: ir.UpdatedBy,
- Expires: ir.Expires,
- Query: ir.Query,
- Note: ir.Note,
- Count: ir.Count,
- ExclusiveCount: ir.ExclusiveCount,
+ ID: ir.ID,
+ Name: ir.Name,
+ UpdatedBy: ir.UpdatedBy,
+ Expires: ir.Expires,
+ Query: ir.Query,
+ Note: ir.Note,
})
}
diff --git a/golden/go/storage/storage.go b/golden/go/storage/storage.go
index 9d56ca9..6eeddf5 100644
--- a/golden/go/storage/storage.go
+++ b/golden/go/storage/storage.go
@@ -234,7 +234,7 @@
// that match the ignore rules in the given ignore store. It also returns the
// ignore rules for later matching.
func FilterIgnored(inputTile *tiling.Tile, ignoreStore ignore.IgnoreStore) (*tiling.Tile, paramtools.ParamMatcher, error) {
- ignores, err := ignoreStore.List(false)
+ ignores, err := ignoreStore.List()
if err != nil {
return nil, nil, fmt.Errorf("Failed to get ignores to filter tile: %s", err)
}
diff --git a/golden/go/summary/summary_test.go b/golden/go/summary/summary_test.go
index 1ef9692..bd22a40 100644
--- a/golden/go/summary/summary_test.go
+++ b/golden/go/summary/summary_test.go
@@ -11,6 +11,7 @@
"go.skia.org/infra/golden/go/blame"
"go.skia.org/infra/golden/go/digest_counter"
"go.skia.org/infra/golden/go/ignore"
+ "go.skia.org/infra/golden/go/ignore/mem_ignorestore"
"go.skia.org/infra/golden/go/mocks"
"go.skia.org/infra/golden/go/storage"
"go.skia.org/infra/golden/go/summary"
@@ -164,7 +165,7 @@
storages := &storage.Storage{
ExpectationsStore: mes,
- IgnoreStore: ignore.NewMemIgnoreStore(),
+ IgnoreStore: mem_ignorestore.New(),
MasterTileBuilder: mocks.NewMockTileBuilderFromTile(t, tile),
NCommits: 50,
}
@@ -178,6 +179,8 @@
Query: "config=565",
}))
+ // TODO(kjlubick): FilterIgnored should be tested elsewhere. This would let us
+ // remove the dependency on MasterTileBuilder and IgnoreStore.
tileWithIgnoreApplied, _, err := storage.FilterIgnored(tile, storages.IgnoreStore)
assert.NoError(t, err)
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 3d8bb0c..6cf8077 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -414,7 +414,9 @@
defer metrics2.FuncTimer().Stop()
w.Header().Set("Content-Type", "application/json")
- ignores, err := wh.Storages.IgnoreStore.List(true)
+ // TODO(kjlubick): these ignore structs used to have counts of how often they were applied
+ // in the file - Fix that after the Storages refactoring.
+ ignores, err := wh.Storages.IgnoreStore.List()
if err != nil {
httputils.ReportError(w, r, err, "Failed to retrieve ignore rules, there may be none.")
return
