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

test(spanner)!: fix data race in spanner integration tests #5276

Merged
merged 2 commits into from Jan 6, 2022
Merged

test(spanner)!: fix data race in spanner integration tests #5276

merged 2 commits into from Jan 6, 2022

Conversation

rahul2393
Copy link
Contributor

  • Update GFELatencyMetricsEnabled flag private to the package, to avoid possibility of data race from customers.
  • Added setter/getter method to update the GFELatencyMetricsEnabled flag.
  • Added lock to make GFELatencyMetricsEnabled thread safe.

Fixes #5272

@rahul2393 rahul2393 requested review from a team as code owners January 5, 2022 05:50
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Jan 5, 2022
olavloite
olavloite reviewed Jan 5, 2022
View reviewed changes
spanner/stats.go Outdated
return view.Register(
GFELatencyView,
GFEHeaderMissingCountView,
)
}

func GetGFELatencyMetricsFlag() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be unexported? And same for the setter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

spanner/stats.go
@@ -267,12 +282,6 @@ func checkCommonTagsGFELatency(t *testing.T, m map[tag.Key]string) {
if !strings.HasPrefix(m[tagKeyClientID], "client") {
t.Fatalf("Incorrect client ID: %v", m[tagKeyClientID])
}
if !strings.HasPrefix(m[tagKeyInstance], "gotest") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch 👍

@olavloite
Copy link
Contributor

By the way: The build error seems to be unrelated to this change. AFAICT, it is caused by a formatting error in the unedited iam/go_mod_tidy_hack.go file.

@codyoss It seems that #5269 introduced a formatting error that was hidden by a different build error in that PR, but that it is a formatting error that only seems to be picked up by Go 1.17. At least, if I run gofmt -s -d on that file using Go 1.16, it thinks it's ok.

olavloite
olavloite approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

If I understand it correctly, the GFE Latency feature had not yet been released. We should make sure that this change then is included in the next release, as it would otherwise be a breaking change (we are removing an exported variable).

@rahul2393
Copy link
Contributor Author

rahul2393 commented Jan 5, 2022 •
edited

No its not yet released so its safe for now to get merged in current release, here is the release PR #5219

@rahul2393 rahul2393 mentioned this pull request Jan 5, 2022
Closed
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 5, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 5, 2022
@rahul2393
Copy link
Contributor Author

@olavloite build passed after changing iam/go_mod_tidy_hack.go and adding the unused exported GFELatencyMetricsEnabled field back to this PR. is it good to merge now?
cc: @codyoss @hengfengli

@rahul2393 rahul2393 changed the title test(spanner): fix data race in spanner integration tests test(spanner)!: fix data race in spanner integration tests Jan 5, 2022
hengfengli
hengfengli approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

@rahul2393 rahul2393 merged commit 22df34b into googleapis:main Jan 6, 2022
@yoshi-code-bot yoshi-code-bot mentioned this pull request Jan 6, 2022
Closed
This was referenced Jan 6, 2022
Closed
Closed
@yoshi-code-bot yoshi-code-bot mentioned this pull request Jan 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hengfengli hengfengli hengfengli approved these changes

@olavloite olavloite olavloite approved these changes

@asthamohta asthamohta asthamohta approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the Spanner API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spanner: TestOCStats_GFE_Latency failed
5 participants
@rahul2393 @olavloite @hengfengli @asthamohta @kokoro-team