Watching Vschema#1624
Conversation
| mu.Lock() | ||
| defer mu.Unlock() | ||
| formal.Keyspaces[keyspace] = kformal | ||
| vschema, err := vindexes.BuildVSchema(formal) |
There was a problem hiding this comment.
The downside of not re-reading everything is that we won't handle keyspace deletes correctly. I'm guessing it's not a big deal since it's pretty rare. Will it work as expected if a new keyspace was added?
|
LGTM |
|
@sougou PTAL at extra commit to clear plan cache on reload. Not sure how to unit test that... |
And unit tests, implementing it for etcd and zk.
One caveat: if the list of keyspaces changes, vtgate won't notice.
That way each implementation doesn't have to do it.
| plr.mu.Lock() | ||
| plr.vschema = vschema | ||
| plr.mu.Unlock() | ||
| plr.plans.Clear() |
There was a problem hiding this comment.
Nit: The clear should happen inside the lock.
There was a problem hiding this comment.
Actually, I think it's more complicated. Let's go with this for now. We'll probably need a more elaborate scheme to prevent race conditions here.
There was a problem hiding this comment.
I agree, it's more complicated. With this, there can be plans built with the new vschema, that get flushed for nothing, but that's it. I didn't want to double-lock, then we have to guarantee ordering and such.
There was a problem hiding this comment.
It's still possible that a builder starts using the old vschema, and we update and clear the plan cache, and then the built plan (based on the older vschema) gets added to the cache. This is because we don't hold the lock on vschema while building the plan. Some brainstorming will be needed.
|
LGTM. One nit. |
3 participants
@sougou I think this works, let me know what you think. Note the WatchVSchema implementations are just cut&paste of the WatchSrvKeyspace implementation.
(internal bug b/27241639 )
This change is