cmd/ejobs: download non-error rows by default This can significantly save time and transient errors with downloading massive results experienced in the past. The users typically do not care about error rows. An option -e flag can be passed to download those as well. Change-Id: Iffc7ff89a317a94fd0522a521b3d4fe48eff9bc5 Reviewed-on: https://go-review.googlesource.com/c/pkgsite-metrics/+/642916 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/cmd/ejobs/main.go b/cmd/ejobs/main.go index d3b98df..9ab8b80 100644 --- a/cmd/ejobs/main.go +++ b/cmd/ejobs/main.go
@@ -49,6 +49,7 @@ minImporters int // for start waitInterval time.Duration // for wait force bool // for results + errs bool // for results outfile string // for results ) @@ -77,11 +78,12 @@ fs.DurationVar(&waitInterval, "i", 0, "display updates at this interval") }, }, - {"results", "[-f] [-o FILE.json] JOBID", + {"results", "[-f] [-e] [-o FILE.json] JOBID", "download results as JSON", doResults, func(fs *flag.FlagSet) { fs.BoolVar(&force, "f", false, "download even if unfinished") + fs.BoolVar(&errs, "e", false, "also download error results (by default, only non-error results are downloaded)") fs.StringVar(&outfile, "o", "", "output filename") }, }, @@ -455,7 +457,7 @@ func doResults(ctx context.Context, args []string) (err error) { if len(args) == 0 { - return errors.New("wrong number of args: want [-f] [-o FILE.json] JOB_ID") + return errors.New("wrong number of args: want [-f] [-e] [-o FILE.json] JOB_ID") } jobID := args[0] ts, err := identityTokenSource(ctx) @@ -470,7 +472,7 @@ if !force && done < job.NumEnqueued { return fmt.Errorf("job not finished (%d/%d completed); use -f for partial results", done, job.NumEnqueued) } - results, err := requestJSON[[]*analysis.Result](ctx, "jobs/results?jobid="+jobID, ts) + results, err := requestJSON[[]*analysis.Result](ctx, fmt.Sprintf("jobs/results?jobid=%s&errors=%t", jobID, errs), ts) if err != nil { return err }
diff --git a/internal/analysis/analysis.go b/internal/analysis/analysis.go index 4deb9a6..d44735e 100644 --- a/internal/analysis/analysis.go +++ b/internal/analysis/analysis.go
@@ -274,14 +274,21 @@ return diags } -func ReadResults(ctx context.Context, c *bigquery.Client, binaryName, binaryVersion, binaryArgs string) (_ []*Result, err error) { +// ReadResults reads non-error rows with binaryName, binaryVersion, and binaryArgs +// from the analysis table of c. If errs is "true", error rows are included as well. +func ReadResults(ctx context.Context, c *bigquery.Client, binaryName, binaryVersion, binaryArgs, errs string) (_ []*Result, err error) { defer derrors.Wrap(&err, "ReadResults") q := bigquery.PartitionQuery{ From: c.FullTableName(TableName), PartitionOn: "module_path, version", - Where: fmt.Sprintf("binary_name='%s' AND binary_version='%s' AND binary_args='%s'", - binaryName, binaryVersion, binaryArgs), - OrderBy: "created_at DESC", + OrderBy: "created_at DESC", + } + if errs == "true" { + q.Where = fmt.Sprintf("binary_name='%s' AND binary_version='%s' AND binary_args='%s'", + binaryName, binaryVersion, binaryArgs) + } else { + q.Where = fmt.Sprintf("binary_name='%s' AND binary_version='%s' AND binary_args='%s' AND error=''", + binaryName, binaryVersion, binaryArgs) } iter, err := c.Query(ctx, q.String()) if err != nil {
diff --git a/internal/worker/jobs.go b/internal/worker/jobs.go index fe0e30e..7401262 100644 --- a/internal/worker/jobs.go +++ b/internal/worker/jobs.go
@@ -4,11 +4,10 @@ // Handlers for jobs. // -// jobs/describe?jobid=xxx describe a job - -// TODO: +// jobs/describe?jobid=xxx describe a job // jobs/list list all jobs -// jobs/cancel?jobid=xxx cancel a job +// jobs/cancel?jobid=xxx cancel a job +// jobs/results?jobid=xxx&errors={true|false} get job results package worker @@ -37,7 +36,8 @@ } jobID := r.FormValue("jobid") - return s.processJobRequest(ctx, w, r.URL.Path, jobID, s.jobDB) + errs := r.FormValue("errors") // for results + return s.processJobRequest(ctx, w, r.URL.Path, jobID, errs, s.jobDB) } type jobDB interface { @@ -47,7 +47,7 @@ ListJobs(context.Context, func(*jobs.Job, time.Time) error) error } -func (s *Server) processJobRequest(ctx context.Context, w io.Writer, path, jobID string, db jobDB) error { +func (s *Server) processJobRequest(ctx context.Context, w io.Writer, path, jobID, errs string, db jobDB) error { path = strings.TrimPrefix(path, "/jobs/") switch path { case "describe": // describe one job @@ -91,7 +91,7 @@ if s.bqClient == nil { return errors.New("bq client is nil") } - results, err := analysis.ReadResults(ctx, s.bqClient, job.Binary, job.BinaryVersion, job.BinaryArgs) + results, err := analysis.ReadResults(ctx, s.bqClient, job.Binary, job.BinaryVersion, job.BinaryArgs, errs) if err != nil { return err }
diff --git a/internal/worker/jobs_test.go b/internal/worker/jobs_test.go index f474f6c..6432a30 100644 --- a/internal/worker/jobs_test.go +++ b/internal/worker/jobs_test.go
@@ -30,7 +30,7 @@ } s := &Server{} var buf bytes.Buffer - if err := s.processJobRequest(ctx, &buf, "/jobs/describe", job.ID(), db); err != nil { + if err := s.processJobRequest(ctx, &buf, "/jobs/describe", job.ID(), "false", db); err != nil { t.Fatal(err) } @@ -42,7 +42,7 @@ t.Errorf("got\n%+v\nwant\n%+v", got, job) } - if err := s.processJobRequest(ctx, &buf, "/jobs/cancel", job.ID(), db); err != nil { + if err := s.processJobRequest(ctx, &buf, "/jobs/cancel", job.ID(), "false", db); err != nil { t.Fatal(err) } @@ -55,7 +55,7 @@ } buf.Reset() - if err := s.processJobRequest(ctx, &buf, "/jobs/list", "", db); err != nil { + if err := s.processJobRequest(ctx, &buf, "/jobs/list", "", "", db); err != nil { t.Fatal(err) } // Don't check for specific output, just make sure there's something