fix: Concurrent read,write to a map#467
Conversation
plugins/source_scheduler_dfs.go
Outdated
| clients = table.Multiplex(client) | ||
| } | ||
| p.metrics.initWithClients(table, clients) | ||
| // p.metrics.initWithClients(table, clients) |
There was a problem hiding this comment.
Isn't this already called outside the Go routine? Can you share the stack trace of the concurrency error?
I might need to look at this in the morning with fresh 👀
Ah got it, clients already started syncing and accessing metrics, is that the cause?
Maybe it only happens when we have many tables
erezrokah
left a comment
There was a problem hiding this comment.
I think this is ok but will take another look tomorrow morning.
One comment is that maybe we can re-use the clients we generate to avoid calling Multiplex again
52a321c to
b2540b1
Compare
hermanschaaf
left a comment
There was a problem hiding this comment.
Looks good - this fixes the bug, but introduces another minor one (left a comment)
plugins/source_scheduler_dfs.go
Outdated
| clients = table.Multiplex(client) | ||
| } | ||
| p.metrics.initWithClients(table, clients) | ||
| // p.metrics.initWithClients(table, clients) |
There was a problem hiding this comment.
| // p.metrics.initWithClients(table, clients) |
Let's just remove the line rather?
| } | ||
| // we do this here to avoid locks so we initial the metrics structure once in the main goroutines | ||
| // and then we can just read from it in the other goroutines concurrently given we are not writing to it. | ||
| p.metrics.initWithClients(table, clients) |
There was a problem hiding this comment.
initWithClients recursively traverses the tables, so this still has the (minor) issue that some tables will be initialized twice. Maybe let's add if table.Parent != nil { to the top of this loop for now so that it's correct, and we can clean up/refactor later?
There was a problem hiding this comment.
oh didn't catch that! Yes, definitely only init parent tables as children will be handled by the initWithClients call.
Follow up on #467 (comment) ---
🤖 I have created a release *beep* *boop* --- ## [1.11.1](v1.11.0...v1.11.1) (2022-12-07) ### Bug Fixes * **codegen:** Column type for slices ([7474c90](7474c90)) * Concurrent read,write to a map ([#467](#467)) ([ebef24a](ebef24a)) * **sentry:** Use HTTPSyncTransport, remove flush ([#465](#465)) ([4d48306](4d48306)) * Skip relations when initializing metrics ([#469](#469)) ([5efe564](5efe564)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This reverts commit ebef24a.
Im not sure how we didn't hit this before but I was able to hit this while working on azure number of times. Prob will need another set of eye to make sure it's good now.