backupinfo: introduce a backupinfo package#82623
backupinfo: introduce a backupinfo package#82623craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
stevendanna
left a comment
There was a problem hiding this comment.
Did an early read through since I'm thinking of also doing some reorganisation.
Overall, I think it makes sense to have a package focused on interacting with the context of a specific backup directory.
It's easy to spend a lot of time fretting about details (is a backup directory and the chain of incrementals related to it its own concept that deserve a name, should we separate the reading of the raw bytes out of the backup from parsing their meaning a bit more, etc). But, this seems like a reasonable first cut to me.
| // conflicts between the starting bytes of a protobuf and these magic bytes. | ||
| func isGZipped(dat []byte) bool { | ||
| func IsGZipped(dat []byte) bool { | ||
| gzipPrefix := []byte("\x1F\x8B\x08") | ||
| return bytes.HasPrefix(dat, gzipPrefix) | ||
| } |
There was a problem hiding this comment.
[future] I could see this moving to somewhere in pkg/util
| filename := fmt.Sprintf("%s_%d_%s", | ||
| backupPartitionDescriptorPrefix, nextPartitionedDescFilenameID, sanitizeLocalityKV(kv)) | ||
| filename := fmt.Sprintf("%s_%d_%s", backupPartitionDescriptorPrefix, | ||
| nextPartitionedDescFilenameID, backupinfo.SanitizeLocalityKV(kv)) |
There was a problem hiding this comment.
[future] We use "Sanitize" in some places to mean "redaction" and then here we use it to mean "make a name suitable to be a filename". We can probably clean up the naming here.
| // is stored if present. It can be found in the name of the backup manifest + | ||
| // this suffix. | ||
| backupManifestChecksumSuffix = "-CHECKSUM" | ||
| BackupManifestChecksumSuffix = "-CHECKSUM" |
There was a problem hiding this comment.
[future,discussion] Some of these being public is a signal that perhaps we need some other methods in this package. That is, eventually do we expect it to be the case that most users of this package don't need to worry about the internal structure of these constants.
2b9cff3 to
0e109ae
Compare
|
@stevendanna I've made you wait a little but this should be RFAL 😃 I agree that there is definitely follow up cleanup that we should undertake including moving some of the constants in |
The backupinfo package contains logic related to interacting with information and metadata describing the backup. After this change we have `backupdest` depending on `backupinfo`. Release note: None
|
@adityamaru - mind filing a GH issue corresponding to this PR? |
|
Perfect, thank you! |
|
TFTR! bors r=stevendanna |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
The backupinfo package contains logic related to interacting
with information and metadata describing the backup. After this
change we have
backupdestdepending onbackupinfo.Release note: None