server: remove support for sticky engines #108185
Conversation
|
This change is |
jbowens
force-pushed
the
stickycleanup
branch
2 times, most recently
from
a0f23e1 to
69ea217
Compare
There was a problem hiding this comment.
Are you sure about this? cockroach demo has this idea of "node restart" (\demo shutdown, \demo restart) which are meant to let people test what happens when a node is temporarily down.
By removing support for sticky engine, a \demo shutdown will be equivalent to wiping the node, and \demo restart will be equivalent to \demo add. This seems like a big feature change. At the very least it needs a release note, and probably it also needs a check-in with a PM.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @itsbilal)
There was a problem hiding this comment.
Are you sure about this?
cockroach demohas this idea of "node restart" (\demo shutdown,\demo restart) which are meant to let people test what happens when a node is temporarily down.By removing support for sticky engine, a
\demo shutdownwill be equivalent to wiping the node, and\demo restartwill be equivalent to\demo add.
This removes support for sticky engines but preserves support for sticky VFSes, which allows cockroach demo to preserve filesystem state between "node restarts." The primary difference is that the restart will now close and reopen the storage engine, rather than preserving ephemeral in-memory engine state across the "node restart."
Reviewable status:
|
gotcha. carry on! |
pkg/server/sticky_vfs.go
Outdated
| // must call CloseAllEngines when they're finished to close the underlying | ||
| // engines. | ||
| // filesystem. The caller should ensure any previous engine accessing the | ||
| // same VFS has already been closed. | ||
| Open(ctx context.Context, cfg *Config, spec base.StoreSpec) (storage.Engine, error) |
There was a problem hiding this comment.
Since the implementation for this method really only grabs the VFS from Get() and doesn't store any other state within the registry itself, thoughts on moving this out of the registry interface and just having a helper that takes a StickyVFSRegistry? Good with it either way, just throwing the idea
Epic: none Release note: none
Remove support for reusing engines from the StickyVFSRegistry. Tests should not depend on ephemeral, in-memory engine state between server restarts, or read closed Engine state. Close cockroachdb#108119. Epic: none Release note: none
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/server/sticky_vfs.go line 44 at r2 (raw file):
Previously, itsbilal (Bilal Akhtar) wrote…
Since the implementation for this method really only grabs the VFS from Get() and doesn't store any other state within the registry itself, thoughts on moving this out of the registry interface and just having a helper that takes a
StickyVFSRegistry? Good with it either way, just throwing the idea
There many fewer Open calls than I thought; Made the change to remove the Open function altogether. Good idea.
There was a problem hiding this comment.
Reviewed 2 of 11 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status:
|
TFTR! bors r=itsbilal |
|
Build succeeded: |
Remove support for reusing engines from the StickyVFSRegistry. Tests should not
depend on ephemeral, in-memory engine state between server restarts, or read
closed Engine state.
Close #108119.