Skip to content
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

sql/rowexec: subject column backfills to admission control #79115

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Mar 31, 2022

kv: add txn helpers that configure admission control

This patch adds the functions DB.TxnWithAdmissionControl() and
kv.NewTxnWithAdmissionControl(), which allow the caller to set the
admission control source and priority. The default variants of these
functions use source OTHER which bypasses admission control.

Release note: None

sql/rowexec: subject column backfills to admission control

This patch subjects column backfills to admission control, using normal
priority.

Release note (bug fix): ALTER TABLE [ADD|DROP] COLUMN are now subject
to admission control, which will prevent these operations from
overloading the storage engine.

@erikgrinaker erikgrinaker requested review from sumeerbhola and a team March 31, 2022 10:28
@erikgrinaker erikgrinaker requested a review from a team as a code owner March 31, 2022 10:28
@erikgrinaker erikgrinaker self-assigned this Mar 31, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker
Copy link
Contributor Author

I see that backups use ROOT_KV rather than FROM_SQL, not sure which one if appropriate here. We should also consider using a lower priority, but existing callers all seem to use normal priority.

@dt
Copy link
Member

dt commented Mar 31, 2022

We should also consider using a lower priority

#79086

This patch adds the functions `DB.TxnWithAdmissionControl()` and
`kv.NewTxnWithAdmissionControl()`, which allow the caller to set the
admission control source and priority. The default variants of these
functions use source `OTHER` which bypasses admission control.

Release note: None
This patch subjects column backfills to admission control, using normal
priority.

Release note (bug fix): `ALTER TABLE [ADD|DROP] COLUMN` are now subject
to admission control, which will prevent these operations from
overloading the storage engine.
@erikgrinaker erikgrinaker force-pushed the backfill-admission-control branch from 7191c20 to 1f47599 Compare March 31, 2022 11:08
@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Mar 31, 2022

We should also consider using a lower priority

#79086

Cool, thanks. I want to backport this, and 21.2 only has low/normal/high priorities, so we may want to keep the normal priority just to be conservative. But yeah, we should move all of these bulk jobs over to a lower priority from 22.1 onwards.

@jordanlewis jordanlewis requested review from a team and dt and removed request for a team March 31, 2022 12:14
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt and @sumeerbhola)

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r2.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @dt)

@rafiss
Copy link
Collaborator

rafiss commented Mar 31, 2022

more of a general comment for the future: it seems like it would be quite beneficial if we had a way to assert/test that admission control was in effect. (i had the same desire for verifying that the TTL job was using admission control)

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Apr 1, 2022

TFTRs!

bors r=ajwerner,sumeerbhola

more of a general comment for the future: it seems like it would be quite beneficial if we had a way to assert/test that admission control was in effect.

Yeah, we need a more comprehensive approach to admission control for internal traffic. I'll write up an issue for this, including testing aspects.

EDIT: #79212.

@craig
Copy link
Contributor

craig bot commented Apr 1, 2022

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Apr 1, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 1d4a1a3 to blathers/backport-release-21.2-79115: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@erikgrinaker erikgrinaker deleted the backfill-admission-control branch May 4, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants