Skip to content

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

Merged
rahul2393 merged 2 commits intogoogleapis:mainfrom
rahul2393:fix-data-race
Jan 6, 2022
Merged

test(spanner)!: fix data race in spanner integration tests#5276
rahul2393 merged 2 commits intogoogleapis:mainfrom
rahul2393:fix-data-race

Conversation

@rahul2393
Copy link
Copy Markdown
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 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
Comment thread spanner/stats.go Outdated
)
}

func GetGFELatencyMetricsFlag() bool {
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread spanner/stats.go
if !strings.HasPrefix(m[tagKeyClientID], "client") {
t.Fatalf("Incorrect client ID: %v", m[tagKeyClientID])
}
if !strings.HasPrefix(m[tagKeyInstance], "gotest") {
Copy link
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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.

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 Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spanner: TestOCStats_GFE_Latency failed

5 participants