backupccl: Cosmetic changes to backup_processor.go to improve clarity.#80991
backupccl: Cosmetic changes to backup_processor.go to improve clarity.#80991craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
017382e to
31a095f
Compare
|
Required tests are passing! |
| type returnedSST struct { | ||
| f BackupManifest_File | ||
| sst []byte | ||
| type returnedSpans struct { |
There was a problem hiding this comment.
I think this should be singular?
There was a problem hiding this comment.
maybe should be called "exportedSpan" actually; "returnedSpan" just sounds like somebody returned a roachpb.Span, but this is much more than that; it is the actual data from that span.
There was a problem hiding this comment.
Changed, but you sure about singular? There's literally a field called completedSpans in the object.
| for i, file := range res.Files { | ||
| f := BackupManifest_File{ | ||
| for i, file := range KVResponse.Files { | ||
| partialFile := BackupManifest_File{ |
There was a problem hiding this comment.
calling this a "file" at all is a tad weird; it is just a some data and metadata that identified the span of keys and time that data came from and how much is in it, and isn't a "file" at all just yet, partial or otherwise (it could be complete when we go to write it, for all we know). it could be less confusing to avoid the extra local var completely here: make the ret returnedSpan here, embed Manifest_File in it with a comment saying "BackupManifest_File conveniently already has all the metadata fields we need to identify a key/time span" and avoid the word file in this block entirely?
There was a problem hiding this comment.
Yeah I like that! How's this look?
| } | ||
| } | ||
|
|
||
| func makeFileSSTSink( |
There was a problem hiding this comment.
I'm not sure that "fileSSTSink" is really telling me anything that "sstSink" didn't, but I also sympathize that it wasn't telling much / enough before.
I think the distinguishing characteristics of this thing are that it takes chunks of exported data (which just so happens to be in sst format), buffers and merges it, and then writes SSTables. So maybe bufferedSSTWriter or something like that?
There was a problem hiding this comment.
To me, calling the object a FileSSTSink highlights that it's not a generic sst sink. It outputs BackupManifest_File objects to the progress channel as one of two core outputs, and takes in a BackupManifest_File as an input in exportedSpan (though we're now more subtly calling that metadata). This isn't something that can be used to buffer any other kind of object, which took me a few hours to totally figure out.
Have I convinced?
dbe8eaa to
0dfb4c6
Compare
Release note: None
|
bors r+ |
|
Build succeeded: |
Release note: None
Renames a few things, and moves the sstsink into its own file.