File store compression #4004
File store compression #4004
Conversation
|
Will take a look in am, thanks! If you want you can rebase on dev to get a clean travis run now. |
neilalexander
force-pushed
the
neil/fscompress2
branch
2 times, most recently
from
1a200eb
to
dacc9bd
Compare
| if mb != nil && fs.fcfg.Compression != NoCompression { | ||
| // We've now reached the end of this message block, if we want | ||
| // to compress blocks then now's the time to do it. | ||
| go mb.recompressOnDiskIfNeeded() |
There was a problem hiding this comment.
Wonder if this is better lazily through sync go routine or when we load the block next?
| // returned and the algorithm defaults to no compression. | ||
| return fmt.Errorf("failed to read existing metadata header: %w", err) | ||
| } | ||
| if meta.Algorithm == alg { |
There was a problem hiding this comment.
If we think this is common should we just peek at this part before loading in the whole block?
There was a problem hiding this comment.
I've added a comment about this, it's really not supposed to be a common case but just means we won't end up rewriting blocks pointlessly if for some reason something does try to recompress with the same algorithm.
| if err := tmpFD.Sync(); err != nil { | ||
| return fmt.Errorf("failed to sync temporary file: %w", err) | ||
| } | ||
| if err := tmpFD.Close(); err != nil { |
There was a problem hiding this comment.
This should be a defer up above so that any exit will always close.
There was a problem hiding this comment.
I've made sure that errors result in clean-up, but I wanted to make sure the descriptor was closed before the rename.
| } | ||
|
|
||
| func (alg StoreCompression) Compress(buf []byte) ([]byte, error) { | ||
| if len(buf) < checksumSize { |
There was a problem hiding this comment.
Probably should be a separate const threshold.
There was a problem hiding this comment.
Since we need to preserve the checksum, the checksumSize seems like it makes sense, as we need to make sure the buffer is big enough to do that.
There was a problem hiding this comment.
What I mean is probably does not make sense if block is below 1-2MB or so.
There was a problem hiding this comment.
I wonder if that might be surprising behaviour, what threshold do you think makes most sense?
There was a problem hiding this comment.
Looking at the constants, maybe defaultSmallBlockSize / 2 makes most sense, which would be 0.5MB today, but I’m also aware of the fact that some of the unit tests set the block size to be deliberately small and by adding in a threshold like this, we may lose out on test coverage of compression by accident.
There was a problem hiding this comment.
Make your best call and we can get this merged in tomorrow.
There was a problem hiding this comment.
I think the best thing to do for now is to leave it as-is so that the test coverage is maximal. If we think that compressing small blocks is hurting performance we can always follow it up later with another PR.
neilalexander
force-pushed
the
neil/fscompress2
branch
2 times, most recently
from
1ac6e8a
to
8a84e05
Compare
neilalexander
force-pushed
the
neil/fscompress2
branch
from
8a84e05
to
4b44db2
Compare
Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander
force-pushed
the
neil/fscompress2
branch
from
4b44db2
to
193a69d
Compare
This PR adds internal support for compression of message blocks in the file store. Message blocks will be compressed when they reach their maximum configured block size and we have moved onto the next block, so as to avoid slowing down on-going writes as far as possible.
Compressed blocks are preambled with a small amount of metadata describing the compression algorithm (although at this point only S2 compression is supported) and compression takes place before encryption (so as to maximise the efficiency of the compression where a stream contains redundant or repeated information in the messages).
This PR does not provide any way to configure compression from the stream level yet (or indeed any other level exposed to clients or operators). It also does not go out of its way to compress old uncompressed blocks after turning on compression unless they are compacted or truncated. These omissions will be addressed in follow-up PRs.
New permutations have been added to the file store tests to also ensure test coverage when compression is enabled.
We should note that, when file store encryption is not in use, storage-level or filesystem-level deduplication or compression (such as those provided by some disk arrays or filesystems like ZFS) may end up being considerably more space-efficient than file store compression would. Conversely, when file store encryption is enabled, enabling compression at the file store level may achieve better ratios.
Signed-off-by: Neil Twigg neil@nats.io