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.

Sign up for GitHub

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

Jump to bottom

sqlstats: simplify transaction latency test #120419

Merged
merged 1 commit into from
Mar 20, 2024
Merged

sqlstats: simplify transaction latency test #120419

merged 1 commit into from
Mar 20, 2024

Conversation

dhartunian
Copy link
Collaborator

Remove need for test case counter which causes a data race.

Fixes: #119580
Epic: None
Release note: None

@dhartunian dhartunian requested review from xinhaoz and a team March 13, 2024 17:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dhartunian dhartunian force-pushed the TestTransactionServiceLatencyOnExtendedProtocol-flake branch from 5d29d14 to 2f48d18 Compare March 13, 2024 23:48
abarganier
abarganier reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@abarganier abarganier 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! 0 of 0 LGTMs obtained (waiting on @dhartunian and @xinhaoz)

pkg/sql/sqlstats/sslocal/sql_stats_test.go line 772 at r1 (raw file):

	q := tc.query
	p := tc.placeholders
	tc.Unlock()

nit: here and below, can we use anonymous functions so that we can defer the unlocks?

Code quote:

	tc.Unlock()

@dhartunian dhartunian force-pushed the TestTransactionServiceLatencyOnExtendedProtocol-flake branch from 2f48d18 to b874060 Compare March 19, 2024 15:09
dhartunian
dhartunian commented Mar 19, 2024
View reviewed changes
Copy link
Collaborator Author

@dhartunian dhartunian 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! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

pkg/sql/sqlstats/sslocal/sql_stats_test.go line 772 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: here and below, can we use anonymous functions so that we can defer the unlocks?

done.

@dhartunian dhartunian force-pushed the TestTransactionServiceLatencyOnExtendedProtocol-flake branch from b874060 to 0d9771e Compare March 19, 2024 15:15
@dhartunian dhartunian added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Mar 20, 2024
abarganier
abarganier approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@abarganier abarganier left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@dhartunian
Copy link
Collaborator Author

TFTRs!

bors r=abarganier,xinhaoz

@craig
Copy link
Contributor

craig bot commented Mar 20, 2024

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 20, 2024

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 20, 2024

Build failed (retrying...):

@rickystewart
Copy link
Collaborator

bors r-

./dev generate bazel

@craig
Copy link
Contributor

craig bot commented Mar 20, 2024

Canceled.

@dhartunian
sqlstats: simplify transaction latency test
b71b3ce 
Remove need for test case counter which causes a data race.

Fixes: cockroachdb#119580
Epic: None
Release note: None
@dhartunian dhartunian force-pushed the TestTransactionServiceLatencyOnExtendedProtocol-flake branch from 0d9771e to b71b3ce Compare March 20, 2024 17:51
@dhartunian
Copy link
Collaborator Author

bors r=abarganier,xinhaoz

@craig
Copy link
Contributor

craig bot commented Mar 20, 2024

Build succeeded:

@craig craig bot merged commit 28a4dae into cockroachdb:master Mar 20, 2024
21 of 22 checks passed
@blathers-crl blathers-crl bot mentioned this pull request Mar 20, 2024
Merged
@abarganier abarganier mentioned this pull request Apr 2, 2024
Closed
This was referenced Apr 16, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abarganier abarganier abarganier approved these changes

@xinhaoz xinhaoz xinhaoz approved these changes

Assignees
No one assigned
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pkg/sql/sqlstats/sslocal/sslocal_test: TestTransactionServiceLatencyOnExtendedProtocol failed
5 participants
@dhartunian @cockroach-teamcity @rickystewart @abarganier @xinhaoz