Support decoding multiplexed RRD streams #7091
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teh-cmc
added
enhancement
emilk
reviewed
Aug 7, 2024
emilk
reviewed
Aug 7, 2024
| pub struct Decoder<R: std::io::Read> { | ||
| version: CrateVersion, | ||
| compression: Compression, | ||
| read: R, | ||
| read: Reader<R>, |
There was a problem hiding this comment.
Do we ever want to support unbuffered reads? Aren't those just strictly slower in almost all cases?
There was a problem hiding this comment.
Not all cases -- if you're reading from an array of bytes, like we do in a bunch of places, adding extra buffering is pure waste of time and space.
| /// This is particularly useful when working with stdio streams. | ||
| /// | ||
| /// If you're not familiar with multiplexed RRD streams, then you probably want to use | ||
| /// [`Decoder::new`] instead. |
There was a problem hiding this comment.
Why do we need both constructors though? They look identical, except one requires a buffered reader.
Isn't it just better if we have one constructor that always handled concatenated streams?
There was a problem hiding this comment.
Three reasons:
- As mentioned above, buffering is a net loss is some real world cases that we depend on today.
- There's non negligible overhead to constantly check for unexpected
FileHeaders, which there's really no reason to be paying for in most cases. - This is a very specific constructor that relies specifically on
std::io::BufReader, as opposed to any type that implementsstd::io::BufRead.
6 tasks
teh-cmc
force-pushed
the
cmc/multiplexed_decodeer
branch
from
9533541 to
951ce6b
Compare
teh-cmc
added a commit
that referenced
this pull request
Aug 8, 2024
You can now do this: ``` cat docs/snippets/all/archetypes/*_rust.rrd | rerun rrd print ``` and this: ``` cat docs/snippets/all/archetypes/*_rust.rrd | rrd merge -o /tmp/all_merged.rrd ``` and this ``` cat docs/snippets/all/archetypes/*_rust.rrd | rerun rrd compact --max-rows 99999999 --max-bytes 999999999 -o /tmp/all_compacted_max.rrd ``` - Part of #7048 - DNM: requires #7091
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR: the following is now possible:
This will of course become more interesting as you build more and more complex CLI pipelines with
rerun rrd(Also fixed some missing buffered io while I was around.)
Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerCHANGELOG.mdand the migration guideTo run all checks from
main, comment on the PR with@rerun-bot full-check.