backupccl: add writeKey method to fileSSTSink #137565
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
pkg/backup/file_sst_sink.go
Outdated
| s.midRow = !endRowKey.Equal(key.Key) | ||
|
|
||
| keyAsRowCount := roachpb.RowCount{ | ||
| DataSize: int64(len(fullKey.Key)) + int64(len(value)), |
There was a problem hiding this comment.
I think there should be some logic here that allows us to set the Rows/IndexEntries here (perhaps not IndexEntries since that column will be removed from backup/restore output). Is there a way to extract out the index from a key so that we can determine if Rows should be incremented?
kev-cao
force-pushed
the
backupccl/fileSSTSink-writeKey
branch
2 times, most recently
from
0e593ec to
cba9923
Compare
To support backup compactions, we need to be able to write MVCC keys to a file sink one key at a time. The current `fileSinkSST` only supports writing KV export responses, or essentially one span at a time. This commit adds support for writing key by key. Epic: none Release note: None
kev-cao
force-pushed
the
backupccl/fileSSTSink-writeKey
branch
from
cba9923 to
4921ba7
Compare
| // If the new span is a contiguous extension of the last span, it is also considered to extend the last span. | ||
| // extendSameSpan determines if an identical span should be considered an extension of the last span. | ||
| // This should only be called after s.midRow has been updated for the last key/span written. | ||
| func (s *fileSSTSink) shouldExtendLastFile( |
There was a problem hiding this comment.
Nit: might just make this a regular non-receiver function like the rest of them below.
| // writeKey writes a single key to the SST file. The key should be the full key. span refers to the span that the key | ||
| // belongs to. start and end are the time bounds of the span being backed up. The writing of each key could | ||
| // potentially split up the span of the previously written key (if both keys were part of the same span and the span | ||
| // had to be split). As a consequence the span that is recorded for the new key is returned, as it may be a subspan of | ||
| // the span passed in for the key. | ||
| // | ||
| // flush should be called after the last key is written to ensure that the SST is written to the destination. |
There was a problem hiding this comment.
One of the biggest hurdles for me writing this was determining some of the implicit assumptions we make about the data, which wasn't laid out clearly in write.
I think this could maybe use some additional elaborations? The behavior I have laid out mentally is as so:
writeKey in its most simplest form will write a key such that the previous key was part of the same span, in which case it just adds to that previous BackupManifest_File. In the event that the previous file has reached the fileSpanByteLimit, the previous file will have its span exclusive end updated to this new key, and then create a new file with the new key as its span start, and the end is left untouched.
Another scenario is writeKey receives a span that contiguously extends the last BackupManifest_File's span (i.e. span.Key == lastFile.Span.EndKey). In that case, we can extend the previous backup manifest file by updating its end key to the new span's end key and continue writing. In the event we are unable to do this because of size constraints, we don't do any extension and just treat the span as a new file.
These are the main cases and what I expect to encounter in a compaction. I did add some guardrails against edge cases, namely where the span precedes the previous BackupManifest_File or if the key being written precedes the last written key. The behavior here is to just flush. From a backup perspective, this could potentially result in BackupManifest_Files overlapping within the same backup. However, I think that's fine as this still creates valid BackupManifest_Files with the correct backing SSTs.
To support backup compactions, we need to be able to write MVCC keys to a file sink one key at a time. The current
fileSinkSSTonly supports writing KV export responses, or essentially one span at a time. This commit adds support for writing key by key.Epic: none
Release note: None