jobs: reduce round trips for control queries #119355

stevendanna · 2024-02-19T08:42:59Z

The APIs provided by the job system makes it very easy for calling code
to inadvertently do a good deal of extra work.

This small PR addresses one such example. The control operations were
all loading the job record twice.

In the newly added rttanalysis test for these operations, a bit of code-re-arrangement gets
a nice reduction in the number of KV batches sent for these queries:

Before:

+9,Jobs/cancel_job
+11,Jobs/pause_job
+11,Jobs/resume_job

After:

+6,Jobs/cancel_job
+8,Jobs/pause_job
+6,Jobs/resume_job

Epic: None

cockroach-teamcity · 2024-02-19T08:43:14Z

This change is

msbutler · 2024-02-20T20:06:35Z

pkg/jobs/registry.go

-	if err != nil {
-		return err
-	}
+	job := Job{registry: r, id: jobID}


instantiating a job struct directly like this could be unsafe if a subsequent method on the job struct assumes the job struct contains some state. It also seems like we do this elsewhere. Yet another job todo would be to create some sort of statelessJob interface on which we can safely call these methods.

Definitely agree having some different types here would be good. I think I'll defer that for now to avoid increasing the blast radius of this change.

msbutler · 2024-02-20T20:35:58Z

pkg/jobs/update.go

+		return fmt.Errorf("job with status %s cannot be requested to be paused", md.Status)
+	}
+	if fn != nil {
+		if err := ju.PauseRequestFuncRunner(ctx, fn, md); err != nil {


according to the jobUpdater doctstring, it is merely responsible for accumulating metadata changes to be persisted to disk, but with this change, the jobUpdater now executes the onPauseRequestFunc, which I think can do much more than update in memory metadata.

This is making it harder for me to distinguish what an updater does and what a jobUpdater does. Could you add some comments to updater which explains what it does and its relation to jobUpdater? Further, could you add some commentary to jobUpdater definition that explains what jobUpdater.PauseRequestFunc and jobUpdater.PauseRequestFuncRunner do?

Hrm. Now i feel like we should just delete this updater/JobUpdater.PauseRequestedWithFunc logic. The only user of it is changefeeds, which use it to update their job status. I think they can update their job status in a much less complicated way. Further, no one uses the PauseRequesterInterface.

we could also land this refactor, and chat with cdc about removing PauseRequestedWithFunc afterwards.

Thanks for taking a second look at this. Given that as far as I can tell no jobs actually implement OnPauseRequest, we can completely remove that method. That allows us to simplify the implementation of PauseRequestedWithFunc such that the caller just gets passed the job updater which I think will fix the semantic problems.

Let's do that in this PR or we will never do it.

msbutler · 2024-02-20T20:38:23Z

pkg/sql/control_jobs.go

@@ -66,30 +67,26 @@ func (n *controlJobsNode) startExec(params runParams) error {
 			return errors.AssertionFailedf("%q: expected *DInt, found %T", jobIDDatum, jobIDDatum)
 		}

-		job, err := reg.LoadJobWithTxn(params.ctx, jobspb.JobID(jobID), params.p.InternalSQLTxn())
-		if err != nil {
+		if err := reg.UpdateJobWithTxn(params.ctx, jobspb.JobID(jobID), params.p.InternalSQLTxn(),


nice cleanup!

msbutler

Nice cleanup!

msbutler · 2024-02-21T14:24:41Z

pkg/jobs/registry.go

@@ -1309,15 +1309,12 @@ func (r *Registry) cancelRequested(ctx context.Context, txn isql.Txn, id jobspb.
 func (r *Registry) PauseRequested(
 	ctx context.Context, txn isql.Txn, id jobspb.JobID, reason string,
 ) error {
-	job, resumer, err := r.getJobFn(ctx, txn, id)
+	job, _, err := r.getJobFn(ctx, txn, id)


nit: you could use LoadJobWIthTxn

:D Yeah, have a commit locally that removes getJobFn completely. I'll push that after merging this.

stevendanna · 2024-02-21T14:39:08Z

TFTR!

bors r=msbutler

craig · 2024-02-21T15:13:55Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2024-02-21T15:18:48Z

Build failed:

Bazel Essential CI (Cockroach)

stevendanna · 2024-02-21T15:30:46Z

bors cancel

stevendanna · 2024-02-21T15:30:56Z

Forgot to push some test fixups.

This adds PAUSE, RESUME, and CANCEL JOB to the rttanalysis tests for jobs. Epic: none Release note: None

Previously, we were loading the job twice during control operations since we were both calling registry.LoadJobWithTxn and Update (which also loads the job). Removing this double load required re-arranging some code so that could be run in the context of the job updater. Epic: None Release note: None

We were marking Progress and updated despite not actually writing anything to progress. Resulting in a little bit of unncessary work. Epic: None Release note: None

stevendanna · 2024-02-22T09:17:12Z

bors r=msbutler

craig · 2024-02-22T10:19:55Z

Build succeeded:

Bazel Essential CI (Cockroach)

118907: build: update patch with bug fix to runtime grunning r=sumeerbhola,rickystewart a=aadityasondhi In our previous version of this patch, we missed two entry points and one exit point into the grunning state of a Goroutine. This led to `grunning.Time()` being non-monotonic. This new patch adds those missing integration points. Fixes #95529. Release note: None 119356: jobs: remove double-load from metrics poller r=msbutler a=stevendanna All but the last commit is #119355 Previously, this code loaded the job twice, once via LoadJobWithTxn and again via the call to Unpaused. Here, we re-arrange the code so that it only loads it once. Epic: none Release note: None 119480: opt: reduce allocations for filter implication with disjunctions r=mgartner a=mgartner This commit reduces allocations in `Implicator.FiltersImplyPredicate` by using stack-allocated arrays when there are five or less adjacent disjunctions in the filter or partial index predicate. This should cover the most common cases of filters and predicates with disjunctions. Epic: None Release note: None 119519: ttljob: skip TestSpanToQueryBoundsCompositeKeys under stress r=fqazi a=annrpom This patch skips TestSpanToQueryBoundsCompositeKeys under stress/stressrace as it often hits OOM issues under these conditions. Epic: none Release note: None Co-authored-by: Aaditya Sondhi <[email protected]> Co-authored-by: Steven Danna <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Annie Pompa <[email protected]>

stevendanna requested review from a team as code owners February 19, 2024 08:43

stevendanna requested review from msbutler and rytaft and removed request for a team February 19, 2024 08:43

stevendanna mentioned this pull request Feb 19, 2024

jobs: remove double-load from metrics poller #119356

Merged

stevendanna force-pushed the pause-cancel-rtt-improvements-1 branch 2 times, most recently from 09cf01e to e2b60f1 Compare February 19, 2024 17:05

msbutler reviewed Feb 20, 2024

View reviewed changes

stevendanna force-pushed the pause-cancel-rtt-improvements-1 branch from e2b60f1 to c078f30 Compare February 20, 2024 23:04

stevendanna requested a review from a team as a code owner February 20, 2024 23:04

stevendanna requested review from rharding6373 and removed request for a team February 20, 2024 23:04

stevendanna force-pushed the pause-cancel-rtt-improvements-1 branch from c078f30 to b70cb4e Compare February 20, 2024 23:30

msbutler approved these changes Feb 21, 2024

View reviewed changes

stevendanna added 4 commits February 21, 2024 21:38

bench/rttanalysis: add JOB control queries

f7897ad

This adds PAUSE, RESUME, and CANCEL JOB to the rttanalysis tests for jobs. Epic: none Release note: None

jobs: remove unnecessary progress write in Unpuased

48c677e

We were marking Progress and updated despite not actually writing anything to progress. Resulting in a little bit of unncessary work. Epic: None Release note: None

jobs: remove OnPauseRequest

09e5cd2

stevendanna force-pushed the pause-cancel-rtt-improvements-1 branch from b70cb4e to 09e5cd2 Compare February 21, 2024 21:39

craig bot merged commit 15b75eb into cockroachdb:master Feb 22, 2024
20 checks passed

stevendanna mentioned this pull request Apr 19, 2024

jobs: crdb_internal.jobs and crdb_internal.system_jobs perform poorly #122687

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jobs: reduce round trips for control queries #119355

jobs: reduce round trips for control queries #119355

stevendanna commented Feb 19, 2024 •

edited

Loading

cockroach-teamcity commented Feb 19, 2024

msbutler Feb 20, 2024

stevendanna Feb 20, 2024 •

edited

Loading

msbutler Feb 20, 2024

msbutler Feb 20, 2024

msbutler Feb 20, 2024 •

edited

Loading

stevendanna Feb 20, 2024

stevendanna Feb 20, 2024

msbutler Feb 20, 2024

msbutler left a comment

msbutler Feb 21, 2024

stevendanna Feb 21, 2024

stevendanna commented Feb 21, 2024

craig bot commented Feb 21, 2024

craig bot commented Feb 21, 2024

stevendanna commented Feb 21, 2024

stevendanna commented Feb 21, 2024

stevendanna commented Feb 22, 2024

craig bot commented Feb 22, 2024

jobs: reduce round trips for control queries #119355

jobs: reduce round trips for control queries #119355

Conversation

stevendanna commented Feb 19, 2024 • edited Loading

cockroach-teamcity commented Feb 19, 2024

Choose a reason for hiding this comment

stevendanna Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msbutler Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendanna commented Feb 21, 2024

craig bot commented Feb 21, 2024

craig bot commented Feb 21, 2024

stevendanna commented Feb 21, 2024

stevendanna commented Feb 21, 2024

stevendanna commented Feb 22, 2024

craig bot commented Feb 22, 2024

stevendanna commented Feb 19, 2024 •

edited

Loading

stevendanna Feb 20, 2024 •

edited

Loading

msbutler Feb 20, 2024 •

edited

Loading