tool: Add db checkpoint command#962
Conversation
|
This change is |
petermattis
left a comment
There was a problem hiding this comment.
All the other tool commands have tests. This deserves one too.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @petermattis)
tool/db.go, line 80 at r1 (raw file):
Long: ` Creates a Pebble checkpoint in the specified destination directory. A checkpoint is a point-in-time snapshot of DB state. Not safe for concurrent
I believe you'll get an error if you try to run this on an in-use store directory. That is slightly different than not safe for concurrent use, as we're providing guardrails. I'd use the language from the other db commands here.
tool/db.go, line 282 at r1 (raw file):
func (d *dbT) runCheckpoint(cmd *cobra.Command, args []string) { db, err := d.openDB(args[0], nonReadOnly{})
Huh? Checkpoint requires a writable DB. I understand why, but I think we can remove that restriction. Let's file an issue about that.
81a0e2c to
855a684
Compare
itsbilal
left a comment
There was a problem hiding this comment.
TFTR! Added a test.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @petermattis)
tool/db.go, line 80 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I believe you'll get an error if you try to run this on an in-use store directory. That is slightly different than not safe for concurrent use, as we're providing guardrails. I'd use the language from the other
dbcommands here.
Done.
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @itsbilal and @jbowens)
checkpoint.go, line 57 at r2 (raw file):
manifestSize := int64(0) if d.mu.versions.manifest != nil { manifestSize = d.mu.versions.manifest.Size()
Nit: might be cleaner to have Writer.Size() return 0 if the receiver is nil.
tool/testdata/db_checkpoint, line 6 at r2 (raw file):
db checkpoint non-existant
Nit: s/existant/existent/g
Adds a command to create a checkpoint of an existing pebble directory, using the db.Checkpoint method. This needs to be exposed to the command line for use in some of Cockroach's roachtests. Also makes a change in Checkpoint() to better handle empty manifests.
855a684 to
972f6cc
Compare
itsbilal
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @jbowens and @petermattis)
checkpoint.go, line 57 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Nit: might be cleaner to have
Writer.Size()return 0 if the receiver isnil.
Done.
tool/db.go, line 282 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Huh? Checkpoint requires a writable DB. I understand why, but I think we can remove that restriction. Let's file an issue about that.
Done: #965
tool/testdata/db_checkpoint, line 6 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Nit:
s/existant/existent/g
Done.
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @jbowens)
Adds a command to create a checkpoint of an existing pebble directory,
using the db.Checkpoint method. This needs to be exposed to the command
line for use in some of Cockroach's roachtests.
Also makes a change in Checkpoint() to better handle empty manifests.