[gold] Preparing to remove datastore-backed ExpectationsStore
Notes to the reviewer:
- PS 1-2 removes IssueExpStoreFactory and replace it with a method on
ExpectationsStore.
- PS 3+ fills out the IssueExpectations implementation on Firestore.
- PS 3+ also makes the Firestore impl use eventbus like the DS
version did
Bug: skia:9090
Change-Id: I32e90bb3d301164344fd9f3a99e50a8978a8a8fb
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/218557
Reviewed-by: Joe Gregorio <jcgregorio@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
diff --git a/golden/cmd/baseline_server/main.go b/golden/cmd/baseline_server/main.go
index 0541180..09205af 100644
--- a/golden/cmd/baseline_server/main.go
+++ b/golden/cmd/baseline_server/main.go
@@ -95,12 +95,12 @@
}
// Set up the cloud expectations store
- expStore, issueExpStoreFactory, err := ds_expstore.New(ds.DS, evt)
+ expStore, err := ds_expstore.DeprecatedNew(ds.DS, evt)
if err != nil {
sklog.Fatalf("Unable to configure cloud expectations store: %s", err)
}
- tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, issueExpStoreFactory, evt)
+ tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, evt)
if err != nil {
sklog.Fatalf("Unable to instantiate tryjob store: %s", err)
}
@@ -127,7 +127,7 @@
}
// Initialize the Baseliner instance from the values set above.
- baseliner, err := gcs_baseliner.New(gsClient, expStore, issueExpStoreFactory, tryjobStore, vcs)
+ baseliner, err := gcs_baseliner.New(gsClient, expStore, tryjobStore, vcs)
if err != nil {
sklog.Fatalf("Error initializing baseliner: %s", err)
}
diff --git a/golden/cmd/sampler/main.go b/golden/cmd/sampler/main.go
index d6b2d36..ab04903 100644
--- a/golden/cmd/sampler/main.go
+++ b/golden/cmd/sampler/main.go
@@ -177,7 +177,7 @@
var expStore expstorage.ExpectationsStore
var err error
- expStore, _, err = ds_expstore.New(ds.DS, evt)
+ expStore, err = ds_expstore.DeprecatedNew(ds.DS, evt)
if err != nil {
sklog.Fatalf("Unable to create cloud expectations store: %s", err)
}
diff --git a/golden/cmd/skiacorrectness/main.go b/golden/cmd/skiacorrectness/main.go
index 4d3531f..dec4fd8 100644
--- a/golden/cmd/skiacorrectness/main.go
+++ b/golden/cmd/skiacorrectness/main.go
@@ -361,41 +361,40 @@
}
// Set up the cloud expectations store
- expStore, issueExpStoreFactory, err := ds_expstore.New(ds.DS, evt)
+ expStore, err := ds_expstore.DeprecatedNew(ds.DS, evt)
if err != nil {
sklog.Fatalf("Unable to configure cloud expectations store: %s", err)
}
- tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, issueExpStoreFactory, evt)
+ tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, evt)
if err != nil {
sklog.Fatalf("Unable to instantiate tryjob store: %s", err)
}
- tryjobMonitor := gerrit_tryjob_monitor.New(tryjobStore, expStore, issueExpStoreFactory, gerritAPI, *siteURL, evt, *authoritative)
+ tryjobMonitor := gerrit_tryjob_monitor.New(tryjobStore, expStore, gerritAPI, *siteURL, evt, *authoritative)
// Initialize the Baseliner instance from the values set above.
- baseliner, err := gcs_baseliner.New(gsClient, expStore, issueExpStoreFactory, tryjobStore, vcs)
+ baseliner, err := gcs_baseliner.New(gsClient, expStore, tryjobStore, vcs)
if err != nil {
sklog.Fatalf("Error initializing baseliner: %s", err)
}
// Extract the site URL
storages := &storage.Storage{
- DiffStore: diffStore,
- ExpectationsStore: expStore,
- IssueExpStoreFactory: issueExpStoreFactory,
- TraceDB: db,
- MasterTileBuilder: masterTileBuilder,
- NCommits: *nCommits,
- EventBus: evt,
- TryjobStore: tryjobStore,
- TryjobMonitor: tryjobMonitor,
- GerritAPI: gerritAPI,
- GCSClient: gsClient,
- VCS: vcs,
- IsAuthoritative: *authoritative,
- SiteURL: *siteURL,
- IsSparseTile: *sparseInput,
- Baseliner: baseliner,
+ DiffStore: diffStore,
+ ExpectationsStore: expStore,
+ TraceDB: db,
+ MasterTileBuilder: masterTileBuilder,
+ NCommits: *nCommits,
+ EventBus: evt,
+ TryjobStore: tryjobStore,
+ TryjobMonitor: tryjobMonitor,
+ GerritAPI: gerritAPI,
+ GCSClient: gsClient,
+ VCS: vcs,
+ IsAuthoritative: *authoritative,
+ SiteURL: *siteURL,
+ IsSparseTile: *sparseInput,
+ Baseliner: baseliner,
}
// Load the whitelist if there is one and disable querying for issues.
diff --git a/golden/go/baseline/gcs_baseliner/baseliner.go b/golden/go/baseline/gcs_baseliner/baseliner.go
index 1f07a32..624c7aa 100644
--- a/golden/go/baseline/gcs_baseliner/baseliner.go
+++ b/golden/go/baseline/gcs_baseliner/baseliner.go
@@ -37,11 +37,10 @@
// GCS and retrieve them. Other packages use it to continuously write expectations to GCS
// as they become available.
type BaselinerImpl struct {
- gStorageClient storage.GCSClient
- expectationsStore expstorage.ExpectationsStore
- issueExpStoreFactory expstorage.IssueExpStoreFactory
- tryjobStore tryjobstore.TryjobStore
- vcs vcsinfo.VCS
+ gStorageClient storage.GCSClient
+ expectationsStore expstorage.ExpectationsStore
+ tryjobStore tryjobstore.TryjobStore
+ vcs vcsinfo.VCS
// mutex protects lastWrittenBaselines, baselineCache and currentTile
mutex sync.RWMutex
@@ -62,7 +61,7 @@
}
// New creates a new instance of baseliner.Baseliner that interacts with baselines in GCS.
-func New(gStorageClient storage.GCSClient, expectationsStore expstorage.ExpectationsStore, issueExpStoreFactory expstorage.IssueExpStoreFactory, tryjobStore tryjobstore.TryjobStore, vcs vcsinfo.VCS) (*BaselinerImpl, error) {
+func New(gStorageClient storage.GCSClient, expectationsStore expstorage.ExpectationsStore, tryjobStore tryjobstore.TryjobStore, vcs vcsinfo.VCS) (*BaselinerImpl, error) {
c, err := lru.New(issueCacheSize)
if err != nil {
return nil, skerr.Fmt("Error allocating cache: %s", err)
@@ -71,7 +70,6 @@
return &BaselinerImpl{
gStorageClient: gStorageClient,
expectationsStore: expectationsStore,
- issueExpStoreFactory: issueExpStoreFactory,
tryjobStore: tryjobStore,
vcs: vcs,
issueBaselineCache: c,
@@ -180,7 +178,7 @@
// PushIssueBaseline implements the baseline.Baseliner interface.
func (b *BaselinerImpl) PushIssueBaseline(issueID int64, tileInfo baseline.TileInfo, dCounter digest_counter.DigestCounter) error {
- issueExpStore := b.issueExpStoreFactory(issueID)
+ issueExpStore := b.expectationsStore.ForIssue(issueID)
exp, err := issueExpStore.Get()
if err != nil {
return skerr.Fmt("Unable to get issue expectations: %s", err)
diff --git a/golden/go/baseline/gcs_baseliner/baseliner_test.go b/golden/go/baseline/gcs_baseliner/baseliner_test.go
index 6ed1fcb..d886fa0 100644
--- a/golden/go/baseline/gcs_baseliner/baseliner_test.go
+++ b/golden/go/baseline/gcs_baseliner/baseliner_test.go
@@ -26,7 +26,7 @@
mgs.On("ReadBaseline", testCommitHash, baseline.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
- baseliner, err := New(mgs, nil, nil, nil, nil)
+ baseliner, err := New(mgs, nil, nil, nil)
assert.NoError(t, err)
b, err := baseliner.FetchBaseline(testCommitHash, baseline.MasterBranch, false)
@@ -78,7 +78,7 @@
// are not live on master yet).
mgs.On("ReadBaseline", "", testIssueID).Return(additionalTriages, nil).Once()
- baseliner, err := New(mgs, nil, nil, nil, nil)
+ baseliner, err := New(mgs, nil, nil, nil)
assert.NoError(t, err)
b, err := baseliner.FetchBaseline(testCommitHash, testIssueID, false)
@@ -124,7 +124,7 @@
// ReadBaseline should only be called once despite multiple requests below
mgs.On("ReadBaseline", testCommitHash, baseline.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
- baseliner, err := New(mgs, nil, nil, nil, nil)
+ baseliner, err := New(mgs, nil, nil, nil)
assert.NoError(t, err)
for i := 0; i < 10; i++ {
@@ -186,7 +186,7 @@
}
}).Return("gs://test-bucket/baselines/foo-baseline.json", nil).Times(3) // once per commit
- baseliner, err := New(mgs, mes, nil, nil, nil)
+ baseliner, err := New(mgs, mes, nil, nil)
assert.NoError(t, err)
b, err := baseliner.PushMasterBaselines(mcs, "")
diff --git a/golden/go/bbstate/bbstate_test.go b/golden/go/bbstate/bbstate_test.go
index 4675417..e0dec3a 100644
--- a/golden/go/bbstate/bbstate_test.go
+++ b/golden/go/bbstate/bbstate_test.go
@@ -14,7 +14,6 @@
"go.skia.org/infra/go/httputils"
"go.skia.org/infra/go/sklog"
"go.skia.org/infra/go/testutils/unittest"
- "go.skia.org/infra/golden/go/expstorage/ds_expstore"
"go.skia.org/infra/golden/go/tryjobstore"
gstorage "google.golang.org/api/storage/v1"
)
@@ -49,9 +48,7 @@
assert.NoError(t, err)
evt := eventbus.New()
- _, expStoreFactory, err := ds_expstore.New(ds.DS, evt)
- assert.NoError(t, err)
- tjStore, err := tryjobstore.NewCloudTryjobStore(dsClient, expStoreFactory, evt)
+ tjStore, err := tryjobstore.NewCloudTryjobStore(dsClient, evt)
assert.NoError(t, err)
gerritAPI, err := gerrit.NewGerrit(gerrit.GERRIT_SKIA_URL, "", httpClient)
diff --git a/golden/go/expstorage/ds_expstore/ds_expstore.go b/golden/go/expstorage/ds_expstore/ds_expstore.go
index fe226e7..d885e33 100644
--- a/golden/go/expstorage/ds_expstore/ds_expstore.go
+++ b/golden/go/expstorage/ds_expstore/ds_expstore.go
@@ -101,13 +101,13 @@
ExpectationsBlob *datastore.Key // key of the blob that stores expectations
}
-// NewCloudExpectationsStore returns an ExpectationsStore implementation based on
+// DeprecatedNew returns an ExpectationsStore implementation based on
// Cloud Datastore for the master branch and a factory to create ExpectationsStore
// instances for Gerrit issues. The factory uses the same datastore client as the
// master store.
-func New(client *datastore.Client, eventBus eventbus.EventBus) (*DSExpStore, expstorage.IssueExpStoreFactory, error) {
+func DeprecatedNew(client *datastore.Client, eventBus eventbus.EventBus) (*DSExpStore, error) {
if client == nil {
- return nil, nil, sklog.FmtErrorf("Received nil for datastore client.")
+ return nil, sklog.FmtErrorf("Received nil for datastore client.")
}
// Create the instance for the master and set the target entities for the
@@ -119,7 +119,7 @@
blobStore := dsutil.NewBlobStore(client, ds.EXPECTATIONS_BLOB_ROOT, ds.EXPECTATIONS_BLOB)
store := &DSExpStore{
- issueID: expstorage.MasterIssueID,
+ issueID: expstorage.MasterBranch,
changeKind: ds.MASTER_EXP_CHANGE,
eventExpChange: expstorage.EV_EXPSTORAGE_CHANGED,
globalEvent: true,
@@ -131,34 +131,33 @@
blobStore: blobStore,
}
- // The factory allows to create an isolated ExpectationStore instance for the
- // given issue.
- factory := func(issueID int64) expstorage.ExpectationsStore {
- summaryKey := ds.NewKey(ds.HELPER_RECENT_KEYS)
- summaryKey.Name = fmt.Sprintf("expstorage-issue-%d", issueID)
- expectationsKey := ds.NewKey(ds.EXPECTATIONS_BLOB_ROOT)
- expectationsKey.Name = fmt.Sprintf("expstorage-expectations-issue-%d", issueID)
- return &DSExpStore{
- issueID: issueID,
- changeKind: ds.TRYJOB_EXP_CHANGE,
- eventExpChange: expstorage.EV_TRYJOB_EXP_CHANGED,
- globalEvent: false,
- client: client,
- eventBus: eventBus,
- summaryKey: summaryKey,
- expectationsKey: expectationsKey,
- recentKeysList: dsutil.NewRecentKeysList(client, summaryKey, dsutil.DefaultConsistencyDelta),
- blobStore: blobStore,
- }
- }
-
// Check the connection to the cloud datastore and if we could load the
// expectations successfully.
_, _, err := store.loadCurrentExpectations(nil)
if err != nil {
- return nil, nil, sklog.FmtErrorf("Error in test call to the cloud datastore: %s", err)
+ return nil, sklog.FmtErrorf("Error in test call to the cloud datastore: %s", err)
}
- return store, factory, nil
+ return store, nil
+}
+
+// ForIssue implements the ExpectationsStore interface.
+func (c *DSExpStore) ForIssue(issueID int64) expstorage.ExpectationsStore {
+ summaryKey := ds.NewKey(ds.HELPER_RECENT_KEYS)
+ summaryKey.Name = fmt.Sprintf("expstorage-issue-%d", issueID)
+ expectationsKey := ds.NewKey(ds.EXPECTATIONS_BLOB_ROOT)
+ expectationsKey.Name = fmt.Sprintf("expstorage-expectations-issue-%d", issueID)
+ return &DSExpStore{
+ issueID: issueID,
+ changeKind: ds.TRYJOB_EXP_CHANGE,
+ eventExpChange: expstorage.EV_TRYJOB_EXP_CHANGED,
+ globalEvent: false,
+ client: c.client,
+ eventBus: c.eventBus,
+ summaryKey: summaryKey,
+ expectationsKey: expectationsKey,
+ recentKeysList: dsutil.NewRecentKeysList(c.client, summaryKey, dsutil.DefaultConsistencyDelta),
+ blobStore: c.blobStore,
+ }
}
// Get implements the ExpectationsStore interface.
diff --git a/golden/go/expstorage/ds_expstore/ds_expstore_test.go b/golden/go/expstorage/ds_expstore/ds_expstore_test.go
index b85b1b2..ac79fab 100644
--- a/golden/go/expstorage/ds_expstore/ds_expstore_test.go
+++ b/golden/go/expstorage/ds_expstore/ds_expstore_test.go
@@ -34,7 +34,7 @@
// Test the DS backed store for master.
masterEventBus := eventbus.New()
- cloudStore, _, err := New(ds.DS, masterEventBus)
+ cloudStore, err := DeprecatedNew(ds.DS, masterEventBus)
assert.NoError(t, err)
testExpectationStore(t, cloudStore, masterEventBus, 0, expstorage.EV_EXPSTORAGE_CHANGED)
testCloudExpstoreClear(t, cloudStore)
@@ -64,10 +64,10 @@
// Test the expectation store for an individual issue.
masterEventBus := eventbus.New()
- _, issueStoreFactory, err := New(ds.DS, masterEventBus)
+ e, err := DeprecatedNew(ds.DS, masterEventBus)
assert.NoError(t, err)
issueID := int64(1234567)
- issueStore := issueStoreFactory(issueID)
+ issueStore := e.ForIssue(issueID)
testExpectationStore(t, issueStore, masterEventBus, issueID, expstorage.EV_TRYJOB_EXP_CHANGED)
testCloudExpstoreClear(t, issueStore.(*DSExpStore))
}
diff --git a/golden/go/expstorage/fs_expstore/FIRESTORE.md b/golden/go/expstorage/fs_expstore/FIRESTORE.md
index 2218bb7..7ca66c8 100644
--- a/golden/go/expstorage/fs_expstore/FIRESTORE.md
+++ b/golden/go/expstorage/fs_expstore/FIRESTORE.md
@@ -11,12 +11,12 @@
it is assumed to be Untriaged.
There is the idea of the MasterExpectations, which is the Expectations belonging to the
-git branch "master". Additionally, there can be smaller BranchExpectations that belong
+git branch "master". Additionally, there can be smaller IssueExpectations that belong
to a ChangeList (CL) and stay separate from the MasterExpectations until the CL lands.
We'd like to be able to do the following:
- - Store and retrieve Expectations (both MasterExpectations and BranchExpectations).
+ - Store and retrieve Expectations (both MasterExpectations and IssueExpectations).
- Update the Label for a (Grouping, Digest).
- Keep an audit record of what user updated the Label for a given (Grouping, Digest).
- Undo a previous change.
@@ -82,7 +82,7 @@
Documents with Issue==0 and assemble them together. The implementation will have an Expectations
map in RAM that acts as a write-through cache.
-BranchExpectations will have their changed Expectations (essentially their delta from the
+IssueExpectations will have their changed Expectations (essentially their delta from the
MasterExpectations) stored in the `expectations` Collection with nonzero
Issue fields. When the tryjob monitor notes that a CL has landed, it can make a transaction
to change all the Issue fields of the associated Documents in the `expectations` Collection to 0.
diff --git a/golden/go/expstorage/fs_expstore/fs_expstore.go b/golden/go/expstorage/fs_expstore/fs_expstore.go
index f19f393..80a0b56 100644
--- a/golden/go/expstorage/fs_expstore/fs_expstore.go
+++ b/golden/go/expstorage/fs_expstore/fs_expstore.go
@@ -6,11 +6,13 @@
"context"
"errors"
"fmt"
+ "strconv"
"sync"
"time"
"cloud.google.com/go/firestore"
"github.com/cenkalti/backoff"
+ "go.skia.org/infra/go/eventbus"
ifirestore "go.skia.org/infra/go/firestore"
"go.skia.org/infra/go/metrics2"
"go.skia.org/infra/go/skerr"
@@ -27,10 +29,6 @@
ReadWrite
)
-const (
- MasterBranch = int64(0)
-)
-
var (
ReadOnlyErr = errors.New("expectationStore is in read-only mode")
)
@@ -62,6 +60,16 @@
mode AccessMode
issue int64 // Gerrit or GitHub issue, or MasterBranch
+ // eventBus allows this Store to communicate with the outside world when
+ // expectations change.
+ eventBus eventbus.EventBus
+ // globalEvent keeps track whether we want to send events within this instance
+ // or on the global eventbus.
+ globalEvent bool
+ // eventExpChange keeps track of which event to fire when the expectations change.
+ // This will be for either the MasterExpectations or for an IssueExpectations.
+ eventExpChange string
+
// cacheMutex protects the write-through cache object.
cacheMutex sync.RWMutex
cache types.Expectations
@@ -99,40 +107,99 @@
LabelAfter types.Label `firestore:"after"`
}
-// New returns a new Store using the given firestore client. The issue param is used
-// to indicate if this Store is configured to read/write the baselines for a given CL
-// or if it is on MasterBranch.
-func New(client *ifirestore.Client, issue int64, mode AccessMode) *Store {
+// New returns a new Store using the given firestore client. The Store will track
+// MasterBranch- see ForIssue() for getting Stores that track ChangeLists.
+func New(client *ifirestore.Client, eventBus eventbus.EventBus, mode AccessMode) *Store {
return &Store{
- client: client,
- issue: issue,
- mode: mode,
+ client: client,
+ eventBus: eventBus,
+ eventExpChange: expstorage.EV_EXPSTORAGE_CHANGED,
+ globalEvent: true,
+ issue: expstorage.MasterBranch,
+ mode: mode,
+ }
+}
+
+// ForIssue implements the ExpectationsStore interface.
+func (f *Store) ForIssue(id int64) expstorage.ExpectationsStore {
+ if id == expstorage.MasterBranch {
+ // This should likely never happen, but if it does, lock the master branch
+ // down to read-only to prevent multiple threads writing to it at once.
+ // One of the core assumptions of this implementation is that there is only
+ // ever one process writing to the master branch.
+ return New(f.client, f.eventBus, ReadOnly)
+ }
+ return &Store{
+ client: f.client,
+ eventBus: f.eventBus,
+ eventExpChange: expstorage.EV_TRYJOB_EXP_CHANGED,
+ globalEvent: false,
+ issue: id,
+ mode: f.mode,
}
}
// Get implements the ExpectationsStore interface.
func (f *Store) Get() (types.Expectations, error) {
- defer metrics2.FuncTimer().Stop()
- f.cacheMutex.RLock()
- defer f.cacheMutex.RUnlock()
- if f.cache == nil {
- c, err := f.loadExpectations()
+ if f.issue == expstorage.MasterBranch {
+ defer metrics2.NewTimer("gold_get_expectations", map[string]string{"master_branch": "true"}).Stop()
+ // On the initial load, we need to make sure we have write-access
+ // to the mutex. Otherwise, read access is sufficient.
+ if f.cache == nil {
+ f.cacheMutex.Lock()
+ defer f.cacheMutex.Unlock()
+ } else {
+ f.cacheMutex.RLock()
+ defer f.cacheMutex.RUnlock()
+ }
+ return f.getMasterExpectations(true)
+ }
+ defer metrics2.NewTimer("gold_get_expectations", map[string]string{"master_branch": "false"}).Stop()
+ return f.getIssueExpectations()
+}
+
+// getMasterExpectations returns an Expectations object which is safe to mutate
+// based on the current state. In cases where caching is fine (e.g. the MasterBranch).
+// If caching it is used, it is expected the caller has taken care of any mutex grabbing.
+func (f *Store) getMasterExpectations(useCache bool) (types.Expectations, error) {
+ if f.cache == nil || !useCache {
+ c, err := f.loadExpectations(expstorage.MasterBranch)
if err != nil {
- return nil, skerr.Fmt("could not load expectations from firestore: %s", err)
+ return nil, skerr.Fmt("could not load master expectations from firestore: %s", err)
+ }
+ if !useCache {
+ return c, nil
}
f.cache = c
}
return f.cache.DeepCopy(), nil
}
-// loadExpectations reads the entire Expectations from the expectationsCollection,
-// matching the configured branch.
-func (f *Store) loadExpectations() (types.Expectations, error) {
+// getIssueExpectations returns an Expectations object which is safe to mutate
+// that has all issue-specific Expectations applied to the master Expectations.
+// It fetches everything from firestore every time, as there could be multiple
+// readers and writers and thus caching isn't safe.
+func (f *Store) getIssueExpectations() (types.Expectations, error) {
+ masterExp, err := f.getMasterExpectations(false)
+ if err != nil {
+ return nil, skerr.Fmt("could not load master expectations: %s", err)
+ }
+ issueExp, err := f.loadExpectations(f.issue)
+ if err != nil {
+ return nil, skerr.Fmt("could not load expectations delta for issue %d from firestore: %s", f.issue, err)
+ }
+ masterExp.MergeExpectations(issueExp)
+ return masterExp, nil
+}
+
+// loadExpectations returns an Expectations object from the expectationsCollection,
+// with all Expectations belonging to the passed in issue (can be MasterBranch).
+func (f *Store) loadExpectations(issue int64) (types.Expectations, error) {
defer metrics2.FuncTimer().Stop()
e := types.Expectations{}
- q := f.client.Collection(expectationsCollection).Where(issueCol, "==", MasterBranch)
+ q := f.client.Collection(expectationsCollection).Where(issueCol, "==", issue)
maxRetries := 3
- err := f.client.IterDocs("loadExpectations", "", q, maxRetries, maxOperationTime, func(doc *firestore.DocumentSnapshot) error {
+ err := f.client.IterDocs("loadExpectations", strconv.FormatInt(issue, 10), q, maxRetries, maxOperationTime, func(doc *firestore.DocumentSnapshot) error {
if doc == nil {
return nil
}
@@ -170,6 +237,13 @@
return now, entries, changes
}()
+ if f.eventBus != nil {
+ f.eventBus.Publish(f.eventExpChange, &expstorage.EventExpectationChange{
+ TestChanges: newExp,
+ IssueID: f.issue,
+ }, f.globalEvent)
+ }
+
// Nothing to add
if len(entries) == 0 {
return nil
@@ -272,7 +346,7 @@
// QueryLog implements the ExpectationsStore interface.
func (f *Store) QueryLog(ctx context.Context, offset, size int, details bool) ([]expstorage.TriageLogEntry, int, error) {
- if offset <= 0 || size <= 0 {
+ if offset < 0 || size <= 0 {
return nil, 0, fmt.Errorf("offset: %d and size: %d must be positive", offset, size)
}
tags := map[string]string{
@@ -285,7 +359,7 @@
// Fetch the records, which have everything except the details.
q := f.client.Collection(triageRecordsCollection).OrderBy(tsCol, firestore.Desc).Offset(offset).Limit(size)
- q = q.Where(issueCol, "==", MasterBranch).Where(committedCol, "==", true)
+ q = q.Where(issueCol, "==", f.issue).Where(committedCol, "==", true)
var rv []expstorage.TriageLogEntry
d := fmt.Sprintf("offset: %d, size %d", offset, size)
err := f.client.IterDocs("query_log", d, q, 3, maxOperationTime, func(doc *firestore.DocumentSnapshot) error {
diff --git a/golden/go/expstorage/fs_expstore/fs_expstore_test.go b/golden/go/expstorage/fs_expstore/fs_expstore_test.go
index 71b89d6..61c07d5 100644
--- a/golden/go/expstorage/fs_expstore/fs_expstore_test.go
+++ b/golden/go/expstorage/fs_expstore/fs_expstore_test.go
@@ -10,6 +10,7 @@
"github.com/google/uuid"
assert "github.com/stretchr/testify/require"
+ "go.skia.org/infra/go/eventbus/mocks"
"go.skia.org/infra/go/firestore"
"go.skia.org/infra/go/testutils/unittest"
"go.skia.org/infra/golden/go/expstorage"
@@ -29,7 +30,7 @@
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
+ f := New(c, nil, ReadWrite)
// Brand new instance should have no expectations
e, err := f.Get()
@@ -73,7 +74,7 @@
// Make sure that if we create a new view, we can read the results
// from the table to make the expectations
- fr := New(c, MasterBranch, ReadOnly)
+ fr := New(c, nil, ReadOnly)
e, err = fr.Get()
assert.NoError(t, err)
assert.Equal(t, expected, e)
@@ -87,7 +88,7 @@
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
+ f := New(c, nil, ReadWrite)
type entry struct {
Grouping types.TestName
@@ -171,7 +172,7 @@
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
+ f := New(c, nil, ReadWrite)
// Write the expectations in two, non-overlapping blocks.
exp1 := makeBigExpectations(0, 16)
@@ -203,7 +204,7 @@
// Make sure that if we create a new view, we can read the results
// from the table to make the expectations
- fr := New(c, MasterBranch, ReadOnly)
+ fr := New(c, nil, ReadOnly)
e, err = fr.Get()
assert.NoError(t, err)
assert.Equal(t, expected, e)
@@ -213,7 +214,7 @@
func TestReadOnly(t *testing.T) {
unittest.SmallTest(t)
- f := New(nil, MasterBranch, ReadOnly)
+ f := New(nil, nil, ReadOnly)
err := f.AddChange(context.Background(), types.Expectations{
data.AlphaTest: {
@@ -230,11 +231,11 @@
unittest.RequiresFirestoreEmulator(t)
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
- ctx := context.Background()
+ f := New(c, nil, ReadWrite)
fillWith4Entries(t, f)
+ ctx := context.Background()
entries, n, err := f.QueryLog(ctx, 0, 100, false)
assert.NoError(t, err)
assert.Equal(t, 4, n) // 4 operations
@@ -306,11 +307,11 @@
unittest.RequiresFirestoreEmulator(t)
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
- ctx := context.Background()
+ f := New(c, nil, ReadWrite)
fillWith4Entries(t, f)
+ ctx := context.Background()
entries, n, err := f.QueryLog(ctx, 0, 100, true)
assert.NoError(t, err)
assert.Equal(t, 4, n) // 4 operations
@@ -356,10 +357,11 @@
unittest.RequiresFirestoreEmulator(t)
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
- ctx := context.Background()
+ f := New(c, nil, ReadWrite)
fillWith4Entries(t, f)
+
+ ctx := context.Background()
entries, n, err := f.QueryLog(ctx, 0, 4, false)
assert.NoError(t, err)
assert.Equal(t, 4, n)
@@ -401,7 +403,7 @@
// Make sure that if we create a new view, we can read the results
// from the table to make the expectations
- fr := New(c, MasterBranch, ReadOnly)
+ fr := New(c, nil, ReadOnly)
exp, err = fr.Get()
assert.NoError(t, err)
assert.Equal(t, expected, exp)
@@ -413,44 +415,299 @@
unittest.RequiresFirestoreEmulator(t)
c := getTestFirestoreInstance(t)
- f := New(c, MasterBranch, ReadWrite)
- ctx := context.Background()
+ f := New(c, nil, ReadWrite)
- _, err := f.UndoChange(ctx, "doesnotexist", "userTwo")
+ _, err := f.UndoChange(context.Background(), "doesnotexist", "userTwo")
assert.Error(t, err)
assert.Contains(t, err.Error(), "not find change")
}
-// TODO(kjlubick): implement tests for branch expectations.
-// func TestBranchExpectationsGet(t *testing.T) {
-// unittest.ManualTest(t)
-// unittest.RequiresFirestoreEmulator(t)
+// TestEventBusAddMaster makes sure proper eventbus signals are sent
+// when changes are made to the master branch.
+func TestEventBusAddMaster(t *testing.T) {
+ unittest.ManualTest(t)
+ unittest.RequiresFirestoreEmulator(t)
-// c := getTestFirestoreInstance(t)
-// m := New(c, MasterBranch, ReadWrite)
-// b := New(c, 117, ReadWrite) // arbitrary branch id
-// ctx := context.Background()
+ meb := &mocks.EventBus{}
+ defer meb.AssertExpectations(t)
-// }
+ c := getTestFirestoreInstance(t)
+ f := New(c, meb, ReadWrite)
-// fillWith4Entries fills a given Store with 4 triaged records of a few digests.
-func fillWith4Entries(t *testing.T, f *Store) {
- assert.NoError(t, f.AddChange(context.Background(), types.Expectations{
+ change1 := types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE,
+ },
+ }
+ change2 := types.Expectations{
+ data.AlphaTest: {
+ data.AlphaBad1Digest: types.NEGATIVE,
+ },
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }
+
+ meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
+ TestChanges: change1,
+ IssueID: expstorage.MasterBranch,
+ }, /*global=*/ true).Once()
+ meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
+ TestChanges: change2,
+ IssueID: expstorage.MasterBranch,
+ }, /*global=*/ true).Once()
+
+ ctx := context.Background()
+ assert.NoError(t, f.AddChange(ctx, change1, userOne))
+ assert.NoError(t, f.AddChange(ctx, change2, userTwo))
+}
+
+// TestEventBusAddIssue makes sure proper eventbus signals are sent
+// when changes are made to an IssueExpectations.
+func TestEventBusAddIssue(t *testing.T) {
+ unittest.ManualTest(t)
+ unittest.RequiresFirestoreEmulator(t)
+
+ meb := &mocks.EventBus{}
+ defer meb.AssertExpectations(t)
+
+ c := getTestFirestoreInstance(t)
+ e := New(c, meb, ReadWrite)
+ issue := int64(117)
+ f := e.ForIssue(issue) // arbitrary issue
+
+ change1 := types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE,
+ },
+ }
+ change2 := types.Expectations{
+ data.AlphaTest: {
+ data.AlphaBad1Digest: types.NEGATIVE,
+ },
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }
+
+ meb.On("Publish", expstorage.EV_TRYJOB_EXP_CHANGED, &expstorage.EventExpectationChange{
+ TestChanges: change1,
+ IssueID: issue,
+ }, /*global=*/ false).Once()
+ meb.On("Publish", expstorage.EV_TRYJOB_EXP_CHANGED, &expstorage.EventExpectationChange{
+ TestChanges: change2,
+ IssueID: issue,
+ }, /*global=*/ false).Once()
+
+ ctx := context.Background()
+ assert.NoError(t, f.AddChange(ctx, change1, userOne))
+ assert.NoError(t, f.AddChange(ctx, change2, userTwo))
+}
+
+// TestEventBusUndo tests that eventbus signals are properly sent during Undo.
+func TestEventBusUndo(t *testing.T) {
+ unittest.ManualTest(t)
+ unittest.RequiresFirestoreEmulator(t)
+
+ meb := &mocks.EventBus{}
+ defer meb.AssertExpectations(t)
+
+ c := getTestFirestoreInstance(t)
+ f := New(c, meb, ReadWrite)
+
+ change := types.Expectations{
data.AlphaTest: {
data.AlphaGood1Digest: types.NEGATIVE,
},
- }, userOne))
- assert.NoError(t, f.AddChange(context.Background(), types.Expectations{
+ }
+ expectedUndo := types.Expectations{
data.AlphaTest: {
- data.AlphaGood1Digest: types.POSITIVE, // overwrites previous value
+ data.AlphaGood1Digest: types.UNTRIAGED,
+ },
+ }
+
+ meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
+ TestChanges: change,
+ IssueID: expstorage.MasterBranch,
+ }, /*global=*/ true).Once()
+ meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
+ TestChanges: expectedUndo,
+ IssueID: expstorage.MasterBranch,
+ }, /*global=*/ true).Once()
+
+ ctx := context.Background()
+ assert.NoError(t, f.AddChange(ctx, change, userOne))
+
+ entries, n, err := f.QueryLog(ctx, 0, 1, false)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, n)
+
+ exp, err := f.UndoChange(ctx, entries[0].ID, userOne)
+ assert.NoError(t, err)
+ assert.Equal(t, expectedUndo, exp)
+}
+
+// TestIssueExpectationsAddGet tests the separation of the MasterExpectations
+// and the IssueExpectations. It starts with a shared history, then
+// adds some expectations to both, before asserting that they are properly dealt
+// with. Specifically, the IssueExpectations should be applied as a delta to
+// the MasterExpectations.
+func TestIssueExpectationsAddGet(t *testing.T) {
+ unittest.ManualTest(t)
+ unittest.RequiresFirestoreEmulator(t)
+
+ c := getTestFirestoreInstance(t)
+ mb := New(c, nil, ReadWrite)
+
+ ctx := context.Background()
+ assert.NoError(t, mb.AddChange(ctx, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.NEGATIVE,
},
}, userTwo))
- assert.NoError(t, f.AddChange(context.Background(), types.Expectations{
+
+ ib := mb.ForIssue(117) // arbitrary issue id
+
+ masterE, err := mb.Get()
+ assert.NoError(t, err)
+ issueE, err := ib.Get()
+ assert.NoError(t, err)
+ assert.Equal(t, masterE, issueE)
+
+ // Add to the IssueExpectations
+ assert.NoError(t, ib.AddChange(ctx, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE, // overwrites previous
+ },
data.BetaTest: {
data.BetaGood1Digest: types.POSITIVE,
},
}, userOne))
- assert.NoError(t, f.AddChange(context.Background(), types.Expectations{
+
+ // Add to the MasterExpectations
+ assert.NoError(t, mb.AddChange(ctx, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaBad1Digest: types.NEGATIVE,
+ },
+ }, userOne))
+
+ masterE, err = mb.Get()
+ assert.NoError(t, err)
+ issueE, err = ib.Get()
+ assert.NoError(t, err)
+
+ // Make sure the IssueExpectations did not leak to the MasterExpectations
+ assert.Equal(t, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.NEGATIVE,
+ data.AlphaBad1Digest: types.NEGATIVE,
+ },
+ }, masterE)
+
+ // Make sure the IssueExpectations are applied on top of the updated
+ // MasterExpectations.
+ assert.Equal(t, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE,
+ data.AlphaBad1Digest: types.NEGATIVE,
+ },
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }, issueE)
+}
+
+// TestIssueExpectationsQueryLog makes sure the QueryLogs interacts
+// with the IssueExpectations as expected. Which is to say, the two
+// logs are separate.
+func TestIssueExpectationsQueryLog(t *testing.T) {
+ unittest.ManualTest(t)
+ unittest.RequiresFirestoreEmulator(t)
+
+ c := getTestFirestoreInstance(t)
+ mb := New(c, nil, ReadWrite)
+
+ ctx := context.Background()
+ assert.NoError(t, mb.AddChange(ctx, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE,
+ },
+ }, userTwo))
+
+ ib := mb.ForIssue(117) // arbitrary issue id
+
+ assert.NoError(t, ib.AddChange(ctx, types.Expectations{
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }, userOne))
+
+ // Make sure the master logs are separate from the issue logs.
+ // request up to 10 to make sure we would get the issue
+ // change (if the filtering was wrong).
+ entries, n, err := mb.QueryLog(ctx, 0, 10, true)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, n)
+
+ now := time.Now()
+ normalizeEntries(t, now, entries)
+ assert.Equal(t, expstorage.TriageLogEntry{
+ ID: "was_random_0",
+ Name: userTwo,
+ TS: now.Unix(),
+ ChangeCount: 1,
+ Details: []expstorage.TriageDetail{
+ {
+ TestName: data.AlphaTest,
+ Digest: data.AlphaGood1Digest,
+ Label: types.POSITIVE.String(),
+ },
+ },
+ }, entries[0])
+
+ // Make sure the issue logs are separate from the master logs.
+ // Unlike when getting the expectations, the issue logs are
+ // *only* those logs that affected this issue. Not, for example,
+ // all the master logs with the issue logs tacked on.
+ entries, n, err = ib.QueryLog(ctx, 0, 10, true)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, n) // only one change on this branch
+
+ normalizeEntries(t, now, entries)
+ assert.Equal(t, expstorage.TriageLogEntry{
+ ID: "was_random_0",
+ Name: userOne,
+ TS: now.Unix(),
+ ChangeCount: 1,
+ Details: []expstorage.TriageDetail{
+ {
+ TestName: data.BetaTest,
+ Digest: data.BetaGood1Digest,
+ Label: types.POSITIVE.String(),
+ },
+ },
+ }, entries[0])
+}
+
+// fillWith4Entries fills a given Store with 4 triaged records of a few digests.
+func fillWith4Entries(t *testing.T, f *Store) {
+ ctx := context.Background()
+ assert.NoError(t, f.AddChange(ctx, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.NEGATIVE,
+ },
+ }, userOne))
+ assert.NoError(t, f.AddChange(ctx, types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE, // overwrites previous value
+ },
+ }, userTwo))
+ assert.NoError(t, f.AddChange(ctx, types.Expectations{
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }, userOne))
+ assert.NoError(t, f.AddChange(ctx, types.Expectations{
data.AlphaTest: {
data.AlphaBad1Digest: types.NEGATIVE,
},
diff --git a/golden/go/expstorage/mem_expstore/mem_expstore.go b/golden/go/expstorage/mem_expstore/mem_expstore.go
index f603d49..1143963 100644
--- a/golden/go/expstorage/mem_expstore/mem_expstore.go
+++ b/golden/go/expstorage/mem_expstore/mem_expstore.go
@@ -30,6 +30,12 @@
return ret
}
+// See ExpectationsStore interface.
+func (m *MemExpectationsStore) ForIssue(id int64) expstorage.ExpectationsStore {
+ sklog.Fatal("MemExpectation store does not support ForIssue.")
+ return nil
+}
+
// Get fulfills the ExpectationsStore interface.
func (m *MemExpectationsStore) Get() (types.Expectations, error) {
m.mutex.RLock()
@@ -47,7 +53,7 @@
if m.eventBus != nil {
m.eventBus.Publish(expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: changedTests,
- IssueID: expstorage.MasterIssueID,
+ IssueID: expstorage.MasterBranch,
}, true)
}
@@ -75,7 +81,7 @@
if m.eventBus != nil {
m.eventBus.Publish(expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: changedDigests,
- IssueID: expstorage.MasterIssueID,
+ IssueID: expstorage.MasterBranch,
}, true)
}
diff --git a/golden/go/expstorage/types.go b/golden/go/expstorage/types.go
index c540458..7060133 100644
--- a/golden/go/expstorage/types.go
+++ b/golden/go/expstorage/types.go
@@ -18,9 +18,9 @@
// for an issue change. It sends an instance of *TryjobExpChange.
EV_TRYJOB_EXP_CHANGED = "expstorage:tryjob-exp-change"
- // MasterIssueID is the value used for IssueID when we dealing with the
- // master branch. Any IssueID < 0 should be ignored.
- MasterIssueID = -1
+ // MasterBranch is the value used for IssueID when we dealing with the
+ // master branch. Any other IssueID < 0 should be ignored.
+ MasterBranch = int64(-1)
)
func init() {
@@ -53,6 +53,12 @@
// undone. The expectations returned are the expectations that were changed,
// with the newly reverted values.
UndoChange(ctx context.Context, changeID, userID string) (types.Expectations, error)
+
+ // Returns a new ExpectationStore that will deal with the Expectations for an issue
+ // with the given id (aka an IssueExpectations). Any expectations sent to the returned
+ // ExpectationStore will be kept separate from the master branch.
+ // This issue id is the Gerrit id or GitHub id.
+ ForIssue(id int64) ExpectationsStore
}
// TriageDetails represents one changed digest and the label that was
@@ -79,6 +85,3 @@
IssueID int64
TestChanges types.Expectations
}
-
-// IssueExpStoreFactory creates an ExpectationsStore instance for the given issue id.
-type IssueExpStoreFactory func(issueID int64) ExpectationsStore
diff --git a/golden/go/goldingestion/tryjob_ingestion.go b/golden/go/goldingestion/tryjob_ingestion.go
index f15ddca..85dbd4d 100644
--- a/golden/go/goldingestion/tryjob_ingestion.go
+++ b/golden/go/goldingestion/tryjob_ingestion.go
@@ -25,7 +25,6 @@
"go.skia.org/infra/go/vcsinfo"
"go.skia.org/infra/golden/go/bbstate"
"go.skia.org/infra/golden/go/config"
- "go.skia.org/infra/golden/go/expstorage/ds_expstore"
"go.skia.org/infra/golden/go/tryjobstore"
"go.skia.org/infra/golden/go/types"
gstorage "google.golang.org/api/storage/v1"
@@ -95,14 +94,8 @@
// bot uploads results. Currently only applies to the Skia repo.
cfgFile := config.ExtraParams[CONFIG_JOB_CFG_FILE]
- _, expStoreFactory, err := ds_expstore.New(ds.DS, eventBus)
- if err != nil {
- return nil, sklog.FmtErrorf("Unable to create cloud expectations store: %s", err)
- }
- sklog.Infof("Cloud Expectations Store created")
-
// Create the cloud tryjob store.
- tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, expStoreFactory, eventBus)
+ tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, eventBus)
if err != nil {
return nil, fmt.Errorf("Error creating tryjob store: %s", err)
}
diff --git a/golden/go/goldingestion/tryjob_ingestion_test.go b/golden/go/goldingestion/tryjob_ingestion_test.go
index b1654e7..689366f 100644
--- a/golden/go/goldingestion/tryjob_ingestion_test.go
+++ b/golden/go/goldingestion/tryjob_ingestion_test.go
@@ -15,7 +15,6 @@
"go.skia.org/infra/go/testutils/unittest"
"go.skia.org/infra/go/util"
"go.skia.org/infra/go/vcsinfo"
- "go.skia.org/infra/golden/go/expstorage/ds_expstore"
"go.skia.org/infra/golden/go/tryjobstore"
)
@@ -75,9 +74,7 @@
// Set up the TryjobStore.
eventBus := eventbus.New()
- _, expStoreFactory, err := ds_expstore.New(ds.DS, eventBus)
- assert.NoError(t, err)
- tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, expStoreFactory, eventBus)
+ tryjobStore, err := tryjobstore.NewCloudTryjobStore(ds.DS, eventBus)
assert.NoError(t, err)
// Map the path of the file to it's content
diff --git a/golden/go/mocks/ExpectationsStore.go b/golden/go/mocks/ExpectationsStore.go
index b71e66c..4baa29d 100644
--- a/golden/go/mocks/ExpectationsStore.go
+++ b/golden/go/mocks/ExpectationsStore.go
@@ -26,6 +26,22 @@
return r0
}
+// ForIssue provides a mock function with given fields: id
+func (_m *ExpectationsStore) ForIssue(id int64) expstorage.ExpectationsStore {
+ ret := _m.Called(id)
+
+ var r0 expstorage.ExpectationsStore
+ if rf, ok := ret.Get(0).(func(int64) expstorage.ExpectationsStore); ok {
+ r0 = rf(id)
+ } else {
+ if ret.Get(0) != nil {
+ r0 = ret.Get(0).(expstorage.ExpectationsStore)
+ }
+ }
+
+ return r0
+}
+
// Get provides a mock function with given fields:
func (_m *ExpectationsStore) Get() (types.Expectations, error) {
ret := _m.Called()
diff --git a/golden/go/search/new_search.go b/golden/go/search/new_search.go
index eb876fd..4cf8891 100644
--- a/golden/go/search/new_search.go
+++ b/golden/go/search/new_search.go
@@ -275,7 +275,7 @@
ret := make(ExpSlice, 0, 2)
if (q != nil) && (q.Issue > 0) {
- issueExpStore := s.storages.IssueExpStoreFactory(q.Issue)
+ issueExpStore := s.storages.ExpectationsStore.ForIssue(q.Issue)
tjExp, err := issueExpStore.Get()
if err != nil {
return nil, sklog.FmtErrorf("Unable to load expectations for issue %d from tryjobstore: %s", q.Issue, err)
diff --git a/golden/go/search/utils_test.go b/golden/go/search/utils_test.go
index ed1f38b..e03c24c 100644
--- a/golden/go/search/utils_test.go
+++ b/golden/go/search/utils_test.go
@@ -154,7 +154,7 @@
tileBuilder := mocks.NewMockTileBuilderFromTile(t, sample.Tile)
eventBus := eventbus.New()
- baseliner, err := gcs_baseliner.New(nil, mes, nil, nil, nil)
+ baseliner, err := gcs_baseliner.New(nil, mes, nil, nil)
assert.NoError(t, err)
storages := &storage.Storage{
diff --git a/golden/go/storage/storage.go b/golden/go/storage/storage.go
index af72248..6d27d30 100644
--- a/golden/go/storage/storage.go
+++ b/golden/go/storage/storage.go
@@ -45,22 +45,21 @@
// Storage is a container struct for the various storage objects we are using.
// It is intended to reduce parameter lists as we pass around storage objects.
type Storage struct {
- DiffStore diff.DiffStore
- ExpectationsStore expstorage.ExpectationsStore
- IssueExpStoreFactory expstorage.IssueExpStoreFactory
- IgnoreStore ignore.IgnoreStore
- TraceDB tracedb.DB
- MasterTileBuilder TileSource
- EventBus eventbus.EventBus
- TryjobStore tryjobstore.TryjobStore
- TryjobMonitor tryjobs.TryjobMonitor
- GerritAPI gerrit.GerritInterface
- GCSClient GCSClient
- Baseliner baseline.Baseliner
- VCS vcsinfo.VCS
- WhiteListQuery paramtools.ParamSet
- IsAuthoritative bool
- SiteURL string
+ DiffStore diff.DiffStore
+ ExpectationsStore expstorage.ExpectationsStore
+ IgnoreStore ignore.IgnoreStore
+ TraceDB tracedb.DB
+ MasterTileBuilder TileSource
+ EventBus eventbus.EventBus
+ TryjobStore tryjobstore.TryjobStore
+ TryjobMonitor tryjobs.TryjobMonitor
+ GerritAPI gerrit.GerritInterface
+ GCSClient GCSClient
+ Baseliner baseline.Baseliner
+ VCS vcsinfo.VCS
+ WhiteListQuery paramtools.ParamSet
+ IsAuthoritative bool
+ SiteURL string
// IsSparseTile indicates that new tiles should be condensed by removing commits that have no data.
IsSparseTile bool
diff --git a/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor.go b/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor.go
index a18cf25..3cbc40a 100644
--- a/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor.go
+++ b/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor.go
@@ -17,29 +17,27 @@
// GerritTryjobMonitor offers a higher level api to handle tryjob-related tasks on top
// of the tryjobstore package.
type GerritTryjobMonitor struct {
- expStore expstorage.ExpectationsStore
- issueExpStoreFactory expstorage.IssueExpStoreFactory
- gerritAPI gerrit.GerritInterface
- tryjobStore tryjobstore.TryjobStore
- siteURL string
- eventBus eventbus.EventBus
- writeGerritMonitor *util.CondMonitor
- isAuthoritative bool
+ expStore expstorage.ExpectationsStore
+ gerritAPI gerrit.GerritInterface
+ tryjobStore tryjobstore.TryjobStore
+ siteURL string
+ eventBus eventbus.EventBus
+ writeGerritMonitor *util.CondMonitor
+ isAuthoritative bool
}
// New creates a new instance of GerritTryjobMonitor.
// siteURL is URL under which the current site it served. It is used to
// generate URLs that are written to Gerrit CLs.
-func New(tryjobStore tryjobstore.TryjobStore, expStore expstorage.ExpectationsStore, iesFactory expstorage.IssueExpStoreFactory, gerritAPI gerrit.GerritInterface, siteURL string, eventBus eventbus.EventBus, isAuthoritative bool) *GerritTryjobMonitor {
+func New(tryjobStore tryjobstore.TryjobStore, expStore expstorage.ExpectationsStore, gerritAPI gerrit.GerritInterface, siteURL string, eventBus eventbus.EventBus, isAuthoritative bool) *GerritTryjobMonitor {
ret := &GerritTryjobMonitor{
- expStore: expStore,
- issueExpStoreFactory: iesFactory,
- tryjobStore: tryjobStore,
- gerritAPI: gerritAPI,
- siteURL: strings.TrimRight(siteURL, "/"),
- eventBus: eventBus,
- writeGerritMonitor: util.NewCondMonitor(1),
- isAuthoritative: isAuthoritative,
+ expStore: expStore,
+ tryjobStore: tryjobStore,
+ gerritAPI: gerritAPI,
+ siteURL: strings.TrimRight(siteURL, "/"),
+ eventBus: eventBus,
+ writeGerritMonitor: util.NewCondMonitor(1),
+ isAuthoritative: isAuthoritative,
}
// Subscribe to events that a tryjob has been updated.
@@ -122,7 +120,7 @@
// CommitIssueBaseline commits the expectations for the given issue to the master baseline.
func (t *GerritTryjobMonitor) CommitIssueBaseline(issueID int64, user string) error {
// Get the issue expecations.
- issueExpStore := t.issueExpStoreFactory(issueID)
+ issueExpStore := t.expStore.ForIssue(issueID)
issueChanges, err := issueExpStore.Get()
if err != nil {
return sklog.FmtErrorf("Unable to retrieve expecations for issue %d: %s", issueID, err)
diff --git a/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor_test.go b/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor_test.go
index d0e5e24..e4f229b 100644
--- a/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor_test.go
+++ b/golden/go/tryjobs/gerrit_tryjob_monitor/gerrit_tryjob_monitor_test.go
@@ -49,7 +49,7 @@
assert.True(t, storeIssue.CommentAdded)
}).Return(nil)
- tryjobMonitor := New(mtjs, nil, nil, mg, siteURL, meb, isAuthoritative)
+ tryjobMonitor := New(mtjs, nil, mg, siteURL, meb, isAuthoritative)
assert.NoError(t, tryjobMonitor.WriteGoldLinkAsComment(mockIssueID))
}
@@ -71,7 +71,7 @@
mtjs.On("GetIssue", mockIssueID, false).Return(nil, nil)
- tryjobMonitor := New(mtjs, nil, nil, mg, siteURL, meb, isAuthoritative)
+ tryjobMonitor := New(mtjs, nil, mg, siteURL, meb, isAuthoritative)
err := tryjobMonitor.WriteGoldLinkAsComment(mockIssueID)
assert.Error(t, err)
diff --git a/golden/go/tryjobstore/tryjobstore.go b/golden/go/tryjobstore/tryjobstore.go
index 5494f83..f4413ef 100644
--- a/golden/go/tryjobstore/tryjobstore.go
+++ b/golden/go/tryjobstore/tryjobstore.go
@@ -14,7 +14,6 @@
"go.skia.org/infra/go/skerr"
"go.skia.org/infra/go/sklog"
"go.skia.org/infra/go/util"
- "go.skia.org/infra/golden/go/expstorage"
"go.skia.org/infra/golden/go/types"
"golang.org/x/sync/errgroup"
)
@@ -111,13 +110,12 @@
// cloudTryjobStore implements the TryjobStore interface on top of cloud datastore.
type cloudTryjobStore struct {
- client *datastore.Client
- eventBus eventbus.EventBus
- expStoreFactory expstorage.IssueExpStoreFactory
+ client *datastore.Client
+ eventBus eventbus.EventBus
}
// NewCloudTryjobStore creates a new instance of TryjobStore based on cloud datastore.
-func NewCloudTryjobStore(client *datastore.Client, expStoreFactory expstorage.IssueExpStoreFactory, eventBus eventbus.EventBus) (TryjobStore, error) {
+func NewCloudTryjobStore(client *datastore.Client, eventBus eventbus.EventBus) (TryjobStore, error) {
if client == nil {
return nil, sklog.FmtErrorf("Received nil for datastore client.")
}
@@ -127,9 +125,8 @@
}
return &cloudTryjobStore{
- client: client,
- eventBus: eventBus,
- expStoreFactory: expStoreFactory,
+ client: client,
+ eventBus: eventBus,
}, nil
}
diff --git a/golden/go/tryjobstore/tryjobstore_test.go b/golden/go/tryjobstore/tryjobstore_test.go
index 87de591..deebf10 100644
--- a/golden/go/tryjobstore/tryjobstore_test.go
+++ b/golden/go/tryjobstore/tryjobstore_test.go
@@ -34,15 +34,15 @@
defer cleanup()
eventBus := eventbus.New()
- _, expStoreFactory, err := ds_expstore.New(ds.DS, eventBus)
+ estore, err := ds_expstore.DeprecatedNew(ds.DS, eventBus)
assert.NoError(t, err)
- store, err := NewCloudTryjobStore(ds.DS, expStoreFactory, eventBus)
+ store, err := NewCloudTryjobStore(ds.DS, eventBus)
assert.NoError(t, err)
- testTryjobStore(t, store, expStoreFactory)
+ testTryjobStore(t, store, estore)
}
-func testTryjobStore(t *testing.T, store TryjobStore, expStoreFactory expstorage.IssueExpStoreFactory) {
+func testTryjobStore(t *testing.T, store TryjobStore, estore expstorage.ExpectationsStore) {
// Add the issue and two tryjobs to the store.
issueID := int64(99)
patchsetID := int64(1099)
@@ -205,7 +205,7 @@
// TODO(kjlubick): assert something with expLogEntries - it is only added to.
expLogEntries := []expstorage.TriageLogEntry{}
userName := "jdoe@example.com"
- expStore := expStoreFactory(issueID)
+ expStore := estore.ForIssue(issueID)
for i := 0; i < 5; i++ {
triageDetails := []expstorage.TriageDetail{}
changes := types.Expectations{}
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 0315e8c..3d8bb0c 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -581,7 +581,7 @@
// in the request, then get the expectations store for the issue.
expStore := wh.Storages.ExpectationsStore
if req.Issue > 0 {
- expStore = wh.Storages.IssueExpStoreFactory(req.Issue)
+ expStore = wh.Storages.ExpectationsStore.ForIssue(req.Issue)
}
// Add the change.
@@ -901,7 +901,7 @@
details := q.Get("details") == "true"
expStore := wh.Storages.ExpectationsStore
if issue > 0 {
- expStore = wh.Storages.IssueExpStoreFactory(issue)
+ expStore = wh.Storages.ExpectationsStore.ForIssue(issue)
}
logEntries, total, err = expStore.QueryLog(r.Context(), offset, size, details)
