[gold] Remove DiffStore dependency from web handlers.
Bug: skia:10582
Change-Id: Ic0eb9c5d6f2d119d103eb4d991bee276796127c6
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/373743
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/golden/cmd/gold_frontend/gold_frontend.go b/golden/cmd/gold_frontend/gold_frontend.go
index 95d33a3..68e988b 100644
--- a/golden/cmd/gold_frontend/gold_frontend.go
+++ b/golden/cmd/gold_frontend/gold_frontend.go
@@ -253,7 +253,7 @@
mustStartExpectationsCleanupProcess(ctx, fsc, expStore, ixr)
- handlers := mustMakeWebHandlers(diffStore, expStore, gsClient, ignoreStore, ixr, reviewSystems, searchAPI, statusWatcher, tileSource, tjs, vcs)
+ handlers := mustMakeWebHandlers(sqlDB, expStore, gsClient, ignoreStore, ixr, reviewSystems, searchAPI, statusWatcher, tileSource, tjs, vcs)
rootRouter := mustMakeRootRouter(fsc, handlers)
@@ -700,10 +700,10 @@
}
// mustMakeWebHandlers returns a new web.Handlers.
-func mustMakeWebHandlers(diffStore diff.DiffStore, expStore expectations.Store, gsClient storage.GCSClient, ignoreStore ignore.Store, ixr *indexer.Indexer, reviewSystems []clstore.ReviewSystem, searchAPI search.SearchAPI, statusWatcher *status.StatusWatcher, tileSource tilesource.TileSource, tjs tjstore.Store, vcs vcsinfo.VCS) *web.Handlers {
+func mustMakeWebHandlers(db *pgxpool.Pool, expStore expectations.Store, gsClient storage.GCSClient, ignoreStore ignore.Store, ixr *indexer.Indexer, reviewSystems []clstore.ReviewSystem, searchAPI search.SearchAPI, statusWatcher *status.StatusWatcher, tileSource tilesource.TileSource, tjs tjstore.Store, vcs vcsinfo.VCS) *web.Handlers {
handlers, err := web.NewHandlers(web.HandlersConfig{
Baseliner: simple_baseliner.New(expStore),
- DiffStore: diffStore,
+ DB: db,
ExpectationsStore: expStore,
GCSClient: gsClient,
IgnoreStore: ignoreStore,
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 84acb92..45793cd 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -18,6 +18,7 @@
"time"
"github.com/gorilla/mux"
+ "github.com/jackc/pgx/v4/pgxpool"
"go.opencensus.io/trace"
"go.skia.org/infra/golden/go/diffstore/common"
search_fe "go.skia.org/infra/golden/go/search/frontend"
@@ -42,6 +43,7 @@
"go.skia.org/infra/golden/go/search"
"go.skia.org/infra/golden/go/search/export"
"go.skia.org/infra/golden/go/search/query"
+ "go.skia.org/infra/golden/go/sql"
"go.skia.org/infra/golden/go/status"
"go.skia.org/infra/golden/go/storage"
"go.skia.org/infra/golden/go/tilesource"
@@ -88,7 +90,7 @@
// HandlersConfig holds the environment needed by the various http handler functions.
type HandlersConfig struct {
Baseliner baseline.BaselineFetcher
- DiffStore diff.DiffStore
+ DB *pgxpool.Pool
ExpectationsStore expectations.Store
GCSClient storage.GCSClient
IgnoreStore ignore.Store
@@ -129,8 +131,8 @@
}
if val == FullFrontEnd {
- if conf.DiffStore == nil {
- return nil, skerr.Fmt("DiffStore cannot be nil")
+ if conf.DB == nil {
+ return nil, skerr.Fmt("DB cannot be nil")
}
if conf.ExpectationsStore == nil {
return nil, skerr.Fmt("ExpectationsStore cannot be nil")
@@ -1113,16 +1115,16 @@
Status: d.Status,
})
remaining := digests[i:]
- diffs, err := wh.DiffStore.Get(r.Context(), d.Digest, remaining)
+ links, err := wh.getLinksBetween(r.Context(), d.Digest, remaining)
if err != nil {
- sklog.Errorf("Failed to calculate differences: %s", err)
- continue
+ httputils.ReportError(w, err, "could not compute diff metrics", http.StatusInternalServerError)
+ return
}
- for otherDigest, dm := range diffs {
+ for otherDigest, distance := range links {
d3.Links = append(d3.Links, Link{
Source: digestIndex[d.Digest],
Target: digestIndex[otherDigest],
- Value: dm.PixelDiffPercent,
+ Value: distance,
})
}
d3.ParamsetByDigest[d.Digest] = idx.GetParamsetSummary(d.Test, d.Digest, types.ExcludeIgnoredTraces)
@@ -1139,6 +1141,47 @@
sendJSONResponse(w, d3)
}
+// getLinksBetween queries the SQL DB for the PercentPixelsDiff between the left digest and
+// the right digests. It returns them in a map.
+func (wh *Handlers) getLinksBetween(ctx context.Context, left types.Digest, right types.DigestSlice) (map[types.Digest]float32, error) {
+ ctx, span := trace.StartSpan(ctx, "getLinksBetween")
+ span.AddAttributes(trace.Int64Attribute("num_right", int64(len(right))))
+ defer span.End()
+ const statement = `
+SELECT encode(right_digest, 'hex'), percent_pixels_diff FROM DiffMetrics
+AS OF SYSTEM TIME '-0.1s'
+WHERE left_digest = $1 AND right_digest IN `
+ arguments := make([]interface{}, 0, len(right)+1)
+ lb, err := sql.DigestToBytes(left)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ arguments = append(arguments, lb)
+ for _, r := range right {
+ rb, err := sql.DigestToBytes(r)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ arguments = append(arguments, rb)
+ }
+ vp := sql.ValuesPlaceholders(len(arguments), 1)
+ rows, err := wh.DB.Query(ctx, statement+vp, arguments...)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ defer rows.Close()
+ rv := map[types.Digest]float32{}
+ for rows.Next() {
+ var rightD types.Digest
+ var linkDistance float32
+ if err := rows.Scan(&rightD, &linkDistance); err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ rv[rightD] = linkDistance
+ }
+ return rv, nil
+}
+
// Node represents a single node in a d3 diagram. Used in ClusterDiffResult.
type Node struct {
Name types.Digest `json:"name"`
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index 14149fa..55aaf51 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -45,6 +45,8 @@
"go.skia.org/infra/golden/go/search"
search_fe "go.skia.org/infra/golden/go/search/frontend"
mock_search "go.skia.org/infra/golden/go/search/mocks"
+ "go.skia.org/infra/golden/go/sql/datakitchensink"
+ "go.skia.org/infra/golden/go/sql/sqltest"
bug_revert "go.skia.org/infra/golden/go/testutils/data_bug_revert"
one_by_five "go.skia.org/infra/golden/go/testutils/data_one_by_five"
data "go.skia.org/infra/golden/go/testutils/data_three_devices"
@@ -2899,6 +2901,51 @@
return buf.Bytes()
}
+func TestGetLinksBetween_SomeDiffMetricsExist_Success(t *testing.T) {
+ unittest.LargeTest(t)
+
+ ctx := context.Background()
+ db := sqltest.NewCockroachDBForTestsWithProductionSchema(ctx, t)
+ require.NoError(t, sqltest.BulkInsertDataTables(ctx, db, datakitchensink.Build()))
+ waitForSystemTime()
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ DB: db,
+ },
+ }
+
+ links, err := wh.getLinksBetween(ctx, datakitchensink.DigestA01Pos, []types.Digest{
+ datakitchensink.DigestA02Pos, datakitchensink.DigestA03Pos, datakitchensink.DigestA05Unt,
+ "0123456789abcdef0123456789abcdef", // not a real digest
+ })
+ require.NoError(t, err)
+ assert.Equal(t, map[types.Digest]float32{
+ datakitchensink.DigestA02Pos: 56.25,
+ datakitchensink.DigestA03Pos: 56.25,
+ datakitchensink.DigestA05Unt: 3.125,
+ }, links)
+}
+
+func TestGetLinksBetween_NoDiffMetricsExist_EmptyMapReturned(t *testing.T) {
+ unittest.LargeTest(t)
+
+ ctx := context.Background()
+ db := sqltest.NewCockroachDBForTestsWithProductionSchema(ctx, t)
+ require.NoError(t, sqltest.BulkInsertDataTables(ctx, db, datakitchensink.Build()))
+ waitForSystemTime()
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ DB: db,
+ },
+ }
+
+ links, err := wh.getLinksBetween(ctx, datakitchensink.DigestA01Pos, []types.Digest{
+ "0123456789abcdef0123456789abcdef", // not a real digest
+ })
+ require.NoError(t, err)
+ assert.Empty(t, links)
+}
+
// Because we are calling our handlers directly, the target URL doesn't matter. The target URL
// would only matter if we were calling into the router, so it knew which handler to call.
const requestURL = "/does/not/matter"
@@ -2988,3 +3035,9 @@
func setID(r *http.Request, id string) *http.Request {
return mux.SetURLVars(r, map[string]string{"id": id})
}
+
+// waitForSystemTime waits for a time greater than the duration mentioned in "AS OF SYSTEM TIME"
+// clauses in queries. This way, the queries will be accurate.
+func waitForSystemTime() {
+ time.Sleep(150 * time.Millisecond)
+}
