[gold] Switch to v2 of diffcalculator worker
Bug: skia:12221
Change-Id: If64e7893018fcb3fceb8dc3c8b3736ad423b87bb
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/434463
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/golden/cmd/diffcalculator/BUILD.bazel b/golden/cmd/diffcalculator/BUILD.bazel
index be869da..a0d55e5 100644
--- a/golden/cmd/diffcalculator/BUILD.bazel
+++ b/golden/cmd/diffcalculator/BUILD.bazel
@@ -10,7 +10,6 @@
"//go/common",
"//go/httputils",
"//go/metrics2",
- "//go/reconnectingmemcached",
"//go/skerr",
"//go/sklog",
"//go/util",
@@ -20,7 +19,6 @@
"//golden/go/sql",
"//golden/go/tracing",
"//golden/go/types",
- "@com_github_bradfitz_gomemcache//memcache",
"@com_github_jackc_pgx_v4//pgxpool",
"@com_google_cloud_go_pubsub//:pubsub",
"@com_google_cloud_go_storage//:storage",
diff --git a/golden/cmd/diffcalculator/diffcalculator.go b/golden/cmd/diffcalculator/diffcalculator.go
index 5c90233..0af060b 100644
--- a/golden/cmd/diffcalculator/diffcalculator.go
+++ b/golden/cmd/diffcalculator/diffcalculator.go
@@ -15,14 +15,12 @@
"cloud.google.com/go/pubsub"
gstorage "cloud.google.com/go/storage"
- "github.com/bradfitz/gomemcache/memcache"
"github.com/jackc/pgx/v4/pgxpool"
"go.opencensus.io/trace"
"go.skia.org/infra/go/common"
"go.skia.org/infra/go/httputils"
"go.skia.org/infra/go/metrics2"
- "go.skia.org/infra/go/reconnectingmemcached"
"go.skia.org/infra/go/skerr"
"go.skia.org/infra/go/sklog"
"go.skia.org/infra/go/util"
@@ -108,9 +106,8 @@
db := mustInitSQLDatabase(ctx, dcc)
gis := mustMakeGCSImageSource(ctx, dcc)
- diffcache := mustMakeDiffCache(ctx, dcc)
sqlProcessor := &processor{
- calculator: worker.New(db, gis, diffcache, dcc.WindowSize),
+ calculator: worker.NewV2(db, gis, dcc.WindowSize),
ackCounter: metrics2.GetCounter("diffcalculator_ack"),
nackCounter: metrics2.GetCounter("diffcalculator_nack"),
}
@@ -179,95 +176,6 @@
return b, skerr.Wrap(err)
}
-const failureReconnectLimit = 100
-
-type memcachedDiffCache struct {
- client reconnectingmemcached.Client
-
- // namespace is the string to add to each key to avoid conflicts with more than one
- // gold instance.
- namespace string
-}
-
-func newMemcacheDiffCache(server, namespace string) (*memcachedDiffCache, error) {
- m := &memcachedDiffCache{
- client: reconnectingmemcached.NewClient(reconnectingmemcached.Options{
- Servers: []string{server},
- Timeout: time.Second,
- MaxIdleConnections: 4,
- AllowedFailuresBeforeHealing: failureReconnectLimit,
- }),
- namespace: namespace,
- }
- return m, m.client.Ping()
-}
-
-func key(namespace string, left, right types.Digest) string {
- return namespace + string(left+"_"+right)
-}
-
-// RemoveAlreadyComputedDiffs implements the DiffCache interface.
-func (m *memcachedDiffCache) RemoveAlreadyComputedDiffs(ctx context.Context, left types.Digest, rightDigests []types.Digest) []types.Digest {
- ctx, span := trace.StartSpan(ctx, "memcached_removeDiffs")
- defer span.End()
- keys := make([]string, 0, len(rightDigests))
- for _, right := range rightDigests {
- if left == right {
- continue // this is never computed
- }
- keys = append(keys, key(m.namespace, left, right))
- }
- alreadyCalculated, ok := m.client.GetMulti(keys)
- if !ok {
- return rightDigests // on an error, assume all need to be queried from DB.
- }
- if len(alreadyCalculated) == len(keys) {
- return nil // common case, everything has been computed already.
- }
- // Go through all the inputs. For each one that is not in the returned value of "already been
- // calculated", add it to return value of "needs lookup/calculation".
- rv := make([]types.Digest, 0, len(rightDigests)-len(alreadyCalculated))
- for _, right := range rightDigests {
- if left == right {
- continue // this is never computed
- }
- if _, ok := alreadyCalculated[key(m.namespace, left, right)]; !ok {
- rv = append(rv, right)
- }
- }
- return rv
-}
-
-// StoreDiffComputed implements the DiffCache interface.
-func (m *memcachedDiffCache) StoreDiffComputed(_ context.Context, left, right types.Digest) {
- m.client.Set(&memcache.Item{
- Key: key(m.namespace, left, right),
- Value: []byte{0x01},
- })
-}
-
-// noopDiffCache pretends the memcached instance always does not have what we are looking up.
-// It is useful for corp-instances where we do not have memcached setup.
-type noopDiffCache struct{}
-
-func (n noopDiffCache) RemoveAlreadyComputedDiffs(_ context.Context, _ types.Digest, right []types.Digest) []types.Digest {
- return right
-}
-
-func (n noopDiffCache) StoreDiffComputed(_ context.Context, _, _ types.Digest) {}
-
-func mustMakeDiffCache(_ context.Context, dcc diffCalculatorConfig) worker.DiffCache {
- if dcc.MemcachedServer == "" || dcc.DiffCacheNamespace == "" {
- sklog.Infof("not using memcached")
- return noopDiffCache{}
- }
- dc, err := newMemcacheDiffCache(dcc.MemcachedServer, dcc.DiffCacheNamespace)
- if err != nil {
- sklog.Fatalf("Could not ping memcached server %s: %s", dcc.MemcachedServer, err)
- }
- return dc
-}
-
func listen(ctx context.Context, dcc diffCalculatorConfig, p *processor) error {
psc, err := pubsub.NewClient(ctx, dcc.PubsubProjectID)
if err != nil {
diff --git a/golden/go/diff/worker/worker2.go b/golden/go/diff/worker/worker2.go
index 9d34183..9e3255f 100644
--- a/golden/go/diff/worker/worker2.go
+++ b/golden/go/diff/worker/worker2.go
@@ -28,11 +28,9 @@
// For tests with fewer than the computeTotalGridCutoff number of digests, we calculate all
// diffs for the test. Above this number, we try to be more clever about calculating a subset
// of useful diffs w/o doing too much work for very tests with very flaky traces.
- // This number was chosen because if there are 100 images, we would need to calculate
- // 100*99/2 = ~5k diffs; It can take up to .5 seconds to compute a diff (most of the time spent
- // decoding) and there are 4 worker goroutines. As such, we expect it to take 5k*.5/4 seconds
- // or about 600 seconds, which is the 10 minute timeout we use.
- computeTotalGridCutoff = 100
+ // This number was chosen based on experimentation with the Skia instance (see also
+ // getCommonAndRecentDigests
+ computeTotalGridCutoff = 300
)
type WorkerImpl2 struct {
