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

upgrades: remove redundant ensureSQLSchemaTelemetrySchedule permanent upgrade #139632

Conversation

shubhamdhama
Copy link
Contributor

During startup, CreateSchemaTelemetrySchedule, which creates the scheduled job to collect schema telemetry, is redundantly invoked twice concurrently: once via schematelemetrycontroller.Controller.Start and also through the ensureSQLSchemaTelemetrySchedule permanent upgrade.

This was identified in #119340, where the permanent upgrade encounters contention and is resolved nearly 30 seconds later causing slow startup of the tenant. Although the exact reason for the prolonged transaction deadlock is unclear, we can still benefit by removing this redundant upgrade.

Informs: #119340
Closes: #130931

Release note: None

Epic: none

@shubhamdhama shubhamdhama requested review from a team as code owners January 23, 2025 12:57
@cockroach-teamcity
Copy link
Member

This change is Reviewable

…nt upgrade

During startup, `CreateSchemaTelemetrySchedule`, which creates the
scheduled job to collect schema telemetry, is redundantly invoked twice
concurrently: once via `schematelemetrycontroller.Controller.Start` and
again through the `ensureSQLSchemaTelemetrySchedule` permanent upgrade.

This was identified in cockroachdb#119340, where the permanent upgrade encounters
contention and is resolved nearly 30 seconds later causing slow startup of
the tenant. Although the exact reason for the prolonged transaction
deadlock is unclear, we can still benefit by removing this redundant
upgrade.

Informs: cockroachdb#119340
Closes: cockroachdb#130931

Release note: None

Epic: none
@shubhamdhama shubhamdhama force-pushed the remove-create-schema-telemetry-schedule-job-permanent-upgrade branch from 5b4225a to f04bdb7 Compare January 23, 2025 16:07
@dt dt requested a review from rafiss January 23, 2025 20:10
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Nice find! It seems like this test also confirms that the schedule is created upon startup:

func TestSchemaTelemetrySchedule(t *testing.T) {

@stevendanna
Copy link
Collaborator

Thanks for taking this on!

Let's keep an eye on this post merge. At test cluster startup, we still have something of a thundering herd of servers trying to create the telemetry schedule. Moving it out of the permanent upgrades means the contention won't delay startup, but it is still a little unfortunate that we have all of these processes spinning trying to create this simple record.

@tbg tbg removed their request for review January 24, 2025 10:32
@shubhamdhama
Copy link
Contributor Author

Sure! I'll keep an eye on this.

bors r=rafiss

@craig craig bot merged commit 8e81611 into cockroachdb:master Jan 24, 2025
22 checks passed
@iskettaneh
Copy link
Contributor

blathers backport 25.1

Copy link

blathers-crl bot commented Feb 5, 2025

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.


Issue #130931: branch-release-25.1.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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.

kv/kvclient/rangefeed: TestRangeFeedIntentResolutionRace failed
5 participants