From e82bfc45164d6b5c8717b0bc92028e1c1e3e278c Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Thu, 11 Jun 2020 13:27:03 -0400 Subject: [PATCH] internal/worker: put NewServer args into a struct It was hard to read the args to NewServer, and now that we have two redis clients, it was error-prone to provide them (order matters). Using a struct effectively lets the caller name the args. Change-Id: I0e2e39e09402031fd21a754961a2685c377c75fc Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/768540 Reviewed-by: Julie Qiu CI-Result: Cloud Build --- cmd/worker/main.go | 15 ++++-- .../testing/integration/integration_test.go | 12 ++++- internal/worker/server.go | 51 ++++++++++--------- internal/worker/server_test.go | 10 +++- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/cmd/worker/main.go b/cmd/worker/main.go index 4dc121e4..72c6d21c 100644 --- a/cmd/worker/main.go +++ b/cmd/worker/main.go @@ -90,9 +90,18 @@ func main() { reportingClient := reportingClient(ctx, cfg) redisHAClient := getHARedis(ctx, cfg) redisCacheClient := getCacheRedis(ctx, cfg) - server, err := worker.NewServer(cfg, db, indexClient, proxyClient, sourceClient, - redisHAClient, redisCacheClient, fetchQueue, reportingClient, - config.TaskIDChangeIntervalWorker, *staticPath) + server, err := worker.NewServer(cfg, worker.ServerConfig{ + DB: db, + IndexClient: indexClient, + ProxyClient: proxyClient, + SourceClient: sourceClient, + RedisHAClient: redisHAClient, + RedisCacheClient: redisCacheClient, + Queue: fetchQueue, + ReportingClient: reportingClient, + TaskIDChangeInterval: config.TaskIDChangeIntervalWorker, + StaticPath: *staticPath, + }) if err != nil { log.Fatal(ctx, err) } diff --git a/internal/testing/integration/integration_test.go b/internal/testing/integration/integration_test.go index 8eeb04e9..4ec5290d 100644 --- a/internal/testing/integration/integration_test.go +++ b/internal/testing/integration/integration_test.go @@ -79,7 +79,17 @@ func TestEndToEndProcessing(t *testing.T) { queue := queue.NewInMemory(ctx, proxyClient, source.NewClient(1*time.Second), testDB, 10, worker.FetchAndUpdateState, nil) - workerServer, err := worker.NewServer(&config.Config{}, testDB, indexClient, proxyClient, source.NewClient(1*time.Second), redisHAClient, redisCacheClient, queue, nil, 10*time.Minute, "../../../content/static") + workerServer, err := worker.NewServer(&config.Config{}, worker.ServerConfig{ + DB: testDB, + IndexClient: indexClient, + ProxyClient: proxyClient, + SourceClient: source.NewClient(1 * time.Second), + RedisHAClient: redisHAClient, + RedisCacheClient: redisCacheClient, + Queue: queue, + TaskIDChangeInterval: 10 * time.Minute, + StaticPath: "../../../content/static", + }) if err != nil { t.Fatal(err) } diff --git a/internal/worker/server.go b/internal/worker/server.go index 64a23cfb..12034664 100644 --- a/internal/worker/server.go +++ b/internal/worker/server.go @@ -51,38 +51,41 @@ type Server struct { indexTemplate *template.Template } -// NewServer creates a new Server with the given dependencies. -func NewServer(cfg *config.Config, - db *postgres.DB, - indexClient *index.Client, - proxyClient *proxy.Client, - sourceClient *source.Client, - redisHAClient *redis.Client, - redisCacheClient *redis.Client, - queue queue.Queue, - reportingClient *errorreporting.Client, - taskIDChangeInterval time.Duration, - staticPath string, -) (_ *Server, err error) { - defer derrors.Wrap(&err, "NewServer(db, ic, pc, q, %q)", staticPath) +// ServerConfig contains everything needed by a Server. +type ServerConfig struct { + DB *postgres.DB + IndexClient *index.Client + ProxyClient *proxy.Client + SourceClient *source.Client + RedisHAClient *redis.Client + RedisCacheClient *redis.Client + Queue queue.Queue + ReportingClient *errorreporting.Client + TaskIDChangeInterval time.Duration + StaticPath string +} - indexTemplate, err := parseTemplate(staticPath) +// NewServer creates a new Server with the given dependencies. +func NewServer(cfg *config.Config, scfg ServerConfig) (_ *Server, err error) { + defer derrors.Wrap(&err, "NewServer(db, %+v)", scfg) + + indexTemplate, err := parseTemplate(scfg.StaticPath) if err != nil { return nil, err } return &Server{ cfg: cfg, - db: db, - indexClient: indexClient, - proxyClient: proxyClient, - sourceClient: sourceClient, - redisHAClient: redisHAClient, - redisCacheClient: redisCacheClient, - queue: queue, - reportingClient: reportingClient, + db: scfg.DB, + indexClient: scfg.IndexClient, + proxyClient: scfg.ProxyClient, + sourceClient: scfg.SourceClient, + redisHAClient: scfg.RedisHAClient, + redisCacheClient: scfg.RedisCacheClient, + queue: scfg.Queue, + reportingClient: scfg.ReportingClient, indexTemplate: indexTemplate, - taskIDChangeInterval: taskIDChangeInterval, + taskIDChangeInterval: scfg.TaskIDChangeInterval, }, nil } diff --git a/internal/worker/server_test.go b/internal/worker/server_test.go index c3cf34bb..c227c786 100644 --- a/internal/worker/server_test.go +++ b/internal/worker/server_test.go @@ -164,8 +164,14 @@ func TestWorker(t *testing.T) { // Use 10 workers to have parallelism consistent with the worker binary. q := queue.NewInMemory(ctx, proxyClient, sourceClient, testDB, 10, FetchAndUpdateState, nil) - s, err := NewServer(&config.Config{}, testDB, indexClient, proxyClient, sourceClient, - nil, nil, q, nil, 10*time.Minute, "") + s, err := NewServer(&config.Config{}, ServerConfig{ + DB: testDB, + IndexClient: indexClient, + ProxyClient: proxyClient, + SourceClient: sourceClient, + Queue: q, + TaskIDChangeInterval: 10 * time.Minute, + }) if err != nil { t.Fatal(err) }