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/contention: introduce contention duration threshold cluster setting #77623

Merged
merged 1 commit into from
Mar 12, 2022

Conversation

Azhng
Copy link
Contributor

@Azhng Azhng commented Mar 10, 2022

This introduces sql.contention.event_store.duration_threshold cluster
setting. This cluster setting specifies the minimum contention duration
to cause the contention events to be collected into
crdb_internal.transaction_contention_events virtual table. This cluster
setting has the default value of 0.

Release justification: low risk high reward changes
Release note (sql change): introduce
sql.contention.event_store.duration_threshold cluster setting. This
cluster setting specifies the minimum contention duration to cause
the contention events to be collected into
crdb_internal.transaction_contention_events virtual table.

@Azhng Azhng requested a review from a team March 10, 2022 18:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Small nit and make sure tests pass, otherwise :lgtm:

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)


pkg/sql/contention/cluster_settings.go, line 44 at r1 (raw file):

	settings.TenantWritable,
	"sql.contention.event_store.duration_threshold",
	"minimum contention duration to cause the contention events to be collected "+

nit: add the unit, e.g. ...duration in milliseconds to cause...

Copy link
Contributor Author

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)


pkg/sql/contention/cluster_settings.go, line 44 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: add the unit, e.g. ...duration in milliseconds to cause...

Hmm it's a bit hard to state a unit here since it's a "duration" cluster setting. It's able to parse the unit from user input. (e.g. user can do '10m' or '10s')

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)


pkg/sql/contention/cluster_settings.go, line 44 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm it's a bit hard to state a unit here since it's a "duration" cluster setting. It's able to parse the unit from user input. (e.g. user can do '10m' or '10s')

Got it, when I saw the 0 I though the user would set just the number without the unit. Sounds good 👍

@Azhng
Copy link
Contributor Author

Azhng commented Mar 11, 2022

TFTR!

bors r=maryliag

@craig
Copy link
Contributor

craig bot commented Mar 11, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 11, 2022

Build failed (retrying...):

@irfansharif
Copy link
Contributor

 pkg/sql/contention/event_store_test.go:142:24: not enough arguments in call to newEventStore

From the failed bors attempts. Is it due to this PR?

bors r-

@craig
Copy link
Contributor

craig bot commented Mar 11, 2022

Canceled.

This introduces sql.contention.event_store.duration_threshold cluster
setting. This cluster setting specifies the minimum contention duration
to cause the contention events to be collected into
crdb_internal.transaction_contention_events virtual table. This cluster
setting has the default value of 0.

Release justification: low risk high reward changes
Release note (sql change): introduce
sql.contention.event_store.duration_threshold cluster setting. This
cluster setting specifies the minimum contention duration to cause
the contention events to be collected into
crdb_internal.transaction_contention_events virtual table.
@Azhng
Copy link
Contributor Author

Azhng commented Mar 11, 2022

Sorry about that, rebased.

@Azhng
Copy link
Contributor Author

Azhng commented Mar 12, 2022

bors r=maryliag

@craig
Copy link
Contributor

craig bot commented Mar 12, 2022

Build succeeded:

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.

4 participants