Fix ArcticDB reading streaming data #1647
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
poodlewars
force-pushed
the
man-read-streaming-2
branch
from
8204c01
to
14bae62
Compare
poodlewars
commented
Jun 28, 2024
| return decode_timeseries_descriptor(hdr, data, begin, end, segment.descriptor()); | ||
| } | ||
|
|
||
| std::optional<TimeseriesDescriptor> decode_timeseries_descriptor_for_incompletes( |
There was a problem hiding this comment.
We could use more abstract naming here as it's a bit weird to refer to incompletes. Like decode_timeseries_descriptor_using_header_stream_descriptor. I settled on the current naming as it makes the purpose of the weird special case clearer for people changing the codec layer (it's a good hint about what they might break).
willdealtry
approved these changes
Jun 28, 2024
poodlewars
added a commit
that referenced
this pull request
Jun 30, 2024
Please see the description on the `master` version of this change, #1647 The implementation is significantly different to the master version, because master is written on top of the new binary encoding work. --------- Co-authored-by: Alex Seaton <[email protected]>
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.
Reference Issues/PRs
Fixes https://github.com/man-group/arcticdb-man/issues/80
What does this implement or fix?
Allows ArcticDB to read incomplete segments written by arcticc tick collectors.
Any other comments?
arcticc has a different append incompletes logic to ArcticDB. It writes a dummy
StreamDescriptoron theTimeseriesDescriptor, and relies entirely on theStreamDescriptorin the segment header. That approach is better than the existing ArcticDB approach, which writes the sameStreamDescriptortwice (directly in the header, and in theTimeseriesDescriptor).ArcticDB was reading this dummy stream descriptor and crashing.
Firstly, this PR changes readers to first check the StreamDescriptor on the header of incompletes, rather than using the one stamped on the
TimeseriesDescriptor. It also adds a backwards compat test for the format.Secondly, and this is not required for the PR to be logically correct, we change writers so that, like arcticc, they do not duplicate the
StreamDescriptor. This is done in 99d0870.Checklist
Checklist for code changes...