jobs,*: read job payload and progress from the job_info table by adityamaru · Pull Request #98608 · cockroachdb/cockroach

adityamaru · 2023-03-14T18:39:47Z

This change touches all the parts of the code that were
relying on the system.jobs table to read the payload and/or
progress corresponding to a job. If the cluster version has advanced
past V23_1JobInfoTableIsBackfilled, every job in the system.jobs table
has a payload and progress entry written to the job_info table.
This change migrates callers to use this new table when reading the
payload and progress of a job.

The most important changes are in the logic that drives crdb_internal.system_jobs,
which in turn drives crdb_internal.jobs and SHOW JOBS. Additionally,
there are changes in how the registry resolves the progress and payload
when loading or resuming a job.

Several tests that read the payload and progress from system.jobs now
rely on crdb_internal.system_jobs to fetch this information from either
the jobs table or job_info table depending on the cluster version.

Fixes: #97762

Release note: None

cockroach-teamcity · 2023-03-14T18:39:57Z

This change is

miretskiy

I really only have nits.

miretskiy · 2023-03-16T14:01:15Z

pkg/ccl/changefeedccl/changefeed_test.go

 		if e, ok := fooLogical.(cdctest.EnterpriseTestFeed); ok {
 			var bytes []byte
-			sqlDB.QueryRow(t, `SELECT payload FROM system.jobs WHERE id=$1`, e.JobID()).Scan(&bytes)
+			sqlDB.QueryRow(t, `SELECT payload FROM "".crdb_internal.system_jobs WHERE id = $1`,


curious: why do we need "" ?

Added a comment to jobs_verification.go and changed most test places to use that query instead.

miretskiy · 2023-03-16T14:15:43Z

pkg/jobs/jobs.go

+		if err != nil {
+			return nil, nil, errors.Wrapf(err, "failed to get payload for job %d", j.ID())
+		}
+		if !exists {


do we intend on making progress optional at some point?

maybe. On job creation we expect the job record to have a valid, non-null progress and payload so unless the user manually deletes the progress for a job from system.job_info we will always have an entry. That said SHOW JOBS goes out of its way to handle the absence of a progress row for a job. So I dunno we should decide if having progress is mandatory or not and then apply that through the system.

pkg/jobs/adopt.go

dt

Nice work!

This change touches all the parts of the code that were relying on the `system.jobs` table to read the payload and/or progress corresponding to a job. If the cluster version has advanced past V23_1JobInfoTableIsBackfilled, every job in the system.jobs table has a payload and progress entry written to the `job_info` table. This change migrates callers to use this new table when reading the payload and progress of a job. The most important changes are in the logic that drives `crdb_internal.system_jobs`, which in turn drives `crdb_internal.jobs` and `SHOW JOBS`. Additionally, there are changes in how the registry resolves the progress and payload when loading or resuming a job. Several tests that read the payload and progress from `system.jobs` now rely on `crdb_internal.system_jobs` to fetch this information from either the jobs table or job_info table depending on the cluster version. Fixes: cockroachdb#97762 Release note: None

adityamaru · 2023-03-17T16:11:19Z

TestDecommission is a flake - #96630
The other stress failure is during test server startup: optInToDiagnosticsStatReporting: setting updated but timed out waiting to read new value so nothing seems wrong with the test per-se. Will investigate separately.

adityamaru · 2023-03-17T16:48:47Z

bors r+

craig · 2023-03-17T17:44:40Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-17T20:55:15Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-18T01:28:45Z

Build failed:

Bazel Essential CI (Cockroach)

adityamaru · 2023-03-18T04:02:32Z

bors retry

craig · 2023-03-18T05:06:08Z

Build succeeded:

Bazel Essential CI (Cockroach)

In cockroachdb#98608 some changes were made to the c2c roachtests that caused them to break. This change fixes the tests by querying the correct virutal table. Fixes: cockroachdb#98973 Fixes: cockroachdb#98969 Fixes: cockroachdb#98962 Informs: cockroachdb#98669 Release note: None

98981: roachtest: fix c2c roachtests that read job payload r=adityamaru a=adityamaru In #98608 some changes were made to the c2c roachtests that caused them to break. This change fixes the tests by querying the correct virutal table that interacts with both `system.jobs` and `system.job_info`. Fixes: #98973 Fixes: #98969 Fixes: #98962 Informs: #98669 Release note: None 99124: kvserver: don't write MinVersion in unit test r=erikgrinaker a=pavelkalinnikov The checkpoint creation code already writes this file. Since this is a unit test, we are not in a situation that this file can not exist (e.g. mixed versions situation), thus we don't need to write this file again. Epic: none Release note: none Co-authored-by: adityamaru <adityamaru@gmail.com> Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

In #98608 some changes were made to the c2c roachtests that caused them to break. This change fixes the tests by querying the correct virutal table. Fixes: #98973 Fixes: #98969 Fixes: #98962 Informs: #98669 Release note: None

adityamaru force-pushed the read-from-jobs-table branch 5 times, most recently from f904791 to 017c4ca Compare March 16, 2023 03:41

adityamaru changed the title ~~[WIP, DNM] jobs,*: read progress and payload from system.job_info~~ jobs,*: read job payload and progress from the job_info table Mar 16, 2023

adityamaru requested review from dt and miretskiy March 16, 2023 03:42

adityamaru marked this pull request as ready for review March 16, 2023 03:43

adityamaru requested review from a team as code owners March 16, 2023 03:43

adityamaru requested a review from a team March 16, 2023 03:43

adityamaru requested a review from a team as a code owner March 16, 2023 03:43

adityamaru requested a review from a team March 16, 2023 03:43

adityamaru requested a review from a team as a code owner March 16, 2023 03:43

adityamaru requested review from a team, herkolategan and renatolabs and removed request for a team March 16, 2023 03:43

adityamaru force-pushed the read-from-jobs-table branch from 017c4ca to 741873f Compare March 16, 2023 13:30

miretskiy approved these changes Mar 16, 2023

View reviewed changes

adityamaru force-pushed the read-from-jobs-table branch from 741873f to 73e06c7 Compare March 16, 2023 15:43

dt approved these changes Mar 16, 2023

View reviewed changes

adityamaru force-pushed the read-from-jobs-table branch 2 times, most recently from 3cde93b to e8c1a09 Compare March 17, 2023 13:08

adityamaru force-pushed the read-from-jobs-table branch from 9bf6a60 to 90a7b66 Compare March 17, 2023 15:27

renatolabs mentioned this pull request Mar 17, 2023

bazci: panic when nogo finds an error #98861

Closed

craig bot merged commit ad4463c into cockroachdb:master Mar 18, 2023

adityamaru deleted the read-from-jobs-table branch March 18, 2023 21:58

This was referenced Mar 19, 2023

roachtest: c2c/tpcc/warehouses=1000/duration=60/cutover=30 failed #98973

Closed

roachtest: c2c/tpcc/warehouses=500/duration=10/cutover=5 failed #98969

Closed

roachtest: c2c/kv0 failed #98962

Closed

roachtest: c2c/BulkOps failed #98669

Closed

adityamaru mentioned this pull request Mar 19, 2023

roachtest: fix c2c roachtests that read job payload #98981

Merged

renatolabs mentioned this pull request Mar 21, 2023

roachtest: backup/mixed-version failed [querying system.job_info: unexpected multiple results] #99101

Closed

miretskiy mentioned this pull request Mar 22, 2023

jobs: Ensure that newly updated queries perform well #99219

Closed

blathers-crl bot mentioned this pull request Mar 22, 2023

release-23.1: roachtest: fix c2c roachtests that read job payload #99222

Merged

Conversation

adityamaru commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Mar 14, 2023

Uh oh!

miretskiy left a comment

Choose a reason for hiding this comment

Uh oh!

miretskiy Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

adityamaru Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

miretskiy Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

adityamaru Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dt left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Mar 17, 2023

Uh oh!

adityamaru commented Mar 17, 2023

Uh oh!

craig bot commented Mar 17, 2023

Uh oh!

craig bot commented Mar 17, 2023

Uh oh!

craig bot commented Mar 18, 2023

Uh oh!

adityamaru commented Mar 18, 2023

Uh oh!

craig bot commented Mar 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adityamaru commented Mar 14, 2023 •

edited

Loading