Skip to content

roachtest: update backup fixtures collection URIs to millisecond precision #146318

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

kev-cao
Copy link
Contributor

@kev-cao kev-cao commented May 7, 2025

The current backup fixture driver creates backups to collection URIs that are formatted based on the current time, accurate to the minute. This causes test flakes when two roachtest fixture runs run at the same minute and create backup schedules that backup to the same collection URI. To avoid this, we update the fixtures to be accurate to the millisecond.

Fixes: #146031, #146030

Release note: None

@kev-cao kev-cao added the backport-25.2.x Flags PRs that need to be backported to 25.2 label May 7, 2025
@kev-cao kev-cao requested a review from a team as a code owner May 7, 2025 19:59
@kev-cao kev-cao requested review from srosenberg and DarrylWong and removed request for a team May 7, 2025 19:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

…ision

The current backup fixture driver creates backups to collection URIs
that are formatted based on the current time, accurate to the minute.
This causes test flakes when two roachtest fixture runs run at the same
minute and create backup schedules that backup to the same collection
URI. To avoid this, we update the fixtures to be accurate to the
millisecond.

Fixes: cockroachdb#146031, cockroachdb#146030

Release note: None
@kev-cao kev-cao force-pushed the roachtest/fixture-millisecond branch from ea8cade to b8f66bd Compare May 7, 2025 19:59
@kev-cao kev-cao requested review from jeffswenson and msbutler May 7, 2025 23:39
Copy link
Collaborator

@herkolategan herkolategan left a comment

Choose a reason for hiding this comment

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

Small change, but a mini roachtest smoke test on TC might still be worth it.

@kev-cao
Copy link
Contributor Author

kev-cao commented May 8, 2025

@herkolategan what sort of smoke tests did you have in mind?

@msbutler
Copy link
Collaborator

msbutler commented May 8, 2025

I think we should land this PR as is. I'm unsure if an acceptance will help with this driver, but i'll chat with Kevin offline.

@msbutler
Copy link
Collaborator

msbutler commented May 8, 2025

oh my b, @herkolategan i misinterpreted your suggestion. Yeah, @kev-cao, would you mind manually running the fixture roachtest to ensure things don't blow up?

@kev-cao
Copy link
Contributor Author

kev-cao commented May 8, 2025

@msbutler oh yup already did and the correct directory was created

@kev-cao
Copy link
Contributor Author

kev-cao commented May 8, 2025

TFTR!

bors r=msbutler

craig bot pushed a commit that referenced this pull request May 8, 2025
145435: sql: add sqlcommenter support r=kyle-a-wong a=kyle-a-wong

commit 1/2:

sql: refactor parser.go to take parser options
Refactored parser functions to use a new ParserOptions type configure various options when parsing sql. There are now two types of Parse functions, Parse*() and Parse*WithOptions().  Parse* will use default parse options and the WithOptions alternative allows specfic options to be provided.

Epic: None
Release note: None

----

commit 2/2:

sql: retain SQL commenter tags
SQL commenter library is used by ORM libraries and customer applications to add application context to the queries using inline SQL comments.

This PR includes the following changes:
- A new cluster setting, `sql.sqlcommenter.enabled`, which retains comments during parsing phase.
- If the comments follow [SQL commenter specification](https://google.github.io/sqlcommenter/spec/#introduction), they are converted to SQL tags.
- Changes to insights stats to also include SQL commenter tags.
- Changes to the crdb_internal.*_execution_insights virtual tables to have   a new `query_tags` column.
- Addition of log tags on the conn_ex context. Each sql commenter tag will be added as a log tag, prefixed with 'querytag-'.

Epic: None
Release note (sql change): Added support for query tagging, which allows users to add query tags to their sql statements via comments. These query tags will be logged as log tags and will be included in all log entries logged during the executing of a sql statement. These tags will also be included in traces. Finally, these tags will be included in the `cluster_execution_insights` and `node_exeuction_insights` virtual tables in a new `query_tags` JSONB column. This feature can be enabled with the `sql.sqlcommenter.enabled` cluster setting and is disabled by default. Comments must adhere to the sqlcommenter spec, as defined here: https://google.github.io/sqlcommenter/spec/


146285: roachtest: init providers in main(), not init() r=tbg a=tbg

Initializing them in an init function means we're calling this method in tests,
where it is likely not useful and may even be implicated in opaque test
timeouts such as [1].

[1]: https://cockroachlabs.slack.com/archives/CJ0H8Q97C/p1746629472748029?thread_ts=1746626113.617579&cid=CJ0H8Q97C

The roachtest job still works[^1]:

<img width="1352" alt="image" src="https://github.com/user-attachments/assets/9233459b-5fd6-4e91-a8a5-deba1177eaab" />

[^1]: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Nightlies_RoachtestNightlyGceBazel/19646638?buildTab=overview&hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true

Epic: none


146318: roachtest: update backup fixtures collection URIs to millisecond precision r=msbutler a=kev-cao

The current backup fixture driver creates backups to collection URIs that are formatted based on the current time, accurate to the minute. This causes test flakes when two roachtest fixture runs run at the same minute and create backup schedules that backup to the same collection URI. To avoid this, we update the fixtures to be accurate to the millisecond.

Fixes: #146031, #146030

Release note: None

Co-authored-by: Chandra Thumuluru <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Kevin Cao <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 8, 2025

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

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

LGTM

@craig craig bot merged commit 3467fc3 into cockroachdb:master May 8, 2025
22 checks passed
Copy link

blathers-crl bot commented May 8, 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 #146030: branch-release-25.2.


🦉 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
backport-25.2.x Flags PRs that need to be backported to 25.2 target-release-25.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: backupFixture/tpcc/warehouses=5000/incrementals=48 failed
5 participants