-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release-24.1: cli/doctor, doctor: use right jobs table, skip dropped descs #124302
Conversation
In #97762, we started writing a job's payload (and progress) information to the `system.jobs_info` table. As a result, we had to change the parts of our code that relied on the `system.jobs` table to use `crdb_internal.system_jobs` instead (since that table would inaccurately report that some `payload`s were `NULL`). This change did not occur for the in-memory representation of the jobs table created by debug doctor -- which can result in missing job false-positives. This patch updates debug doctor's representation of the jobs table by referring to `crdb_internal.system_jobs` instead. Epic: none Fixes: #122675 Release note: None
54816a0
to
7932203
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom, @blathers-crl[bot], @fqazi, @nameisbhaskar, and @vidit-bhat)
pkg/sql/doctor/doctor_test.go
line 200 at r2 (raw file):
}, valid: true, expected: "Examining 1 descriptors and 0 namespace entries...\n",
nit: just noticed this. why are we removing the expected error check?
pkg/sql/doctor/doctor_test.go
line 419 at r2 (raw file):
}, valid: true, expected: "Examining 2 descriptors and 1 namespace entries...\n",
nit: just noticed this. why are we removing the expected error check?
@rafiss they are both dropped: cockroach/pkg/sql/doctor/doctor_test.go Line 195 in 7932203
cockroach/pkg/sql/doctor/doctor_test.go Line 407 in 7932203
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok thanks! we should just make sure that the validation it tested is covered by another test case
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom, @blathers-crl[bot], @fqazi, @nameisbhaskar, and @vidit-bhat)
✅ we are covered |
Backport 2/2 commits from #123298 on behalf of @annrpom.
/cc @cockroachdb/release
cli/doctor: doctor should read from the right jobs table
In #97762, we started writing a job's payload (and progress)
information to the
system.jobs_info
table. As a result, wehad to change the parts of our code that relied on the
system.jobs
table to use
crdb_internal.system_jobs
instead (since that tablewould inaccurately report that some
payload
s wereNULL
).This change did not occur for the in-memory representation of the jobs
table created by debug doctor -- which can result in missing job
false-positives. This patch updates debug doctor's representation of
the jobs table by referring to
crdb_internal.system_jobs
instead.Epic: none
Fixes: #122675
Release note: None
doctor: skip validation for dropped descriptors
In some cases, dropped descriptors appear in our
system.descriptors
table with dangling job mutations without an associated job.
This patch teaches debug doctor examine to skip validation
on such dropped descriptors.
Epic: none
Fixes: #123477
Fixes: #122956
Release note: none
Release justification: non-production change to debugging tool