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

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

Merged
merged 2 commits into from Jan 6, 2022

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Jan 5, 2022

  • 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 Jan 5, 2022
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Jan 5, 2022
spanner/stats.go Outdated
return view.Register(
GFELatencyView,
GFEHeaderMissingCountView,
)
}

func GetGFELatencyMetricsFlag() bool {
Copy link
Contributor

@olavloite olavloite Jan 5, 2022

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

@rahul2393 rahul2393 Jan 5, 2022

Choose a reason for hiding this comment

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

done

@@ -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

@olavloite olavloite Jan 5, 2022

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

olavloite commented Jan 5, 2022

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.

Copy link
Contributor

@olavloite olavloite left a comment

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

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

@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

rahul2393 commented Jan 5, 2022

@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
Copy link
Contributor

@hengfengli hengfengli left a comment

LGTM.

@rahul2393 rahul2393 merged commit 22df34b into googleapis:main Jan 6, 2022
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Cloud Spanner API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: TestOCStats_GFE_Latency failed
5 participants