RUMM-2134 Write events to files in TLV format #841
Conversation
There was a problem hiding this comment.
I really like the idea of using TLV, it's a nice enhancement to the originally proposed format 👍. It should be also simpler to reason about - we will start with only one BlockType and add more when iterating on V2 ✨.
My biggest worry is on introducing InputStream as part of this refactoring. I left deeper explanation in the comment. LMKWDYT 🙌.
| internal extension Data { | ||
| /// Returns a Data block in Type-Lenght-Value format. | ||
| /// | ||
| /// A block follow TLV with bytes aligned such as: | ||
| /// | ||
| /// +- 2 bytes -+- 4 bytes -+- n bytes -| | ||
| /// | block type | block size (n) | block data | | ||
| /// +------------+----------------+------------+ | ||
| /// | ||
| /// - Parameters: | ||
| /// - type: The data type | ||
| /// - data: The data | ||
| /// - Returns: a byte sequence in TLV format. | ||
| static func block(_ type: BlockType, data: Data) -> Data { | ||
| return DataBlock(type: type, data: data).serialize() | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this extension needed 💭🤔? I can only see its usage in tests, not in the production code. Also, as we're using Foundation type (Data) to only attach a domain-specific (core) function to, so it doesn't seem to bring much improvement over using DataBlock(type:data:) directly, WDYT?
There was a problem hiding this comment.
Yea, not really needed, it was more for convenience for future change. I will remove it!
Could be cool to leverage a pattern like AlamofireExtended in the future, so we can hide any extension behind a dd property!
There was a problem hiding this comment.
I like the dd idea 👌. We use something similar in tracing tests to cast OT interfaces to our DD domain, but it could be used even more like you suggest 👍.
| XCTAssertEqual(blocks.first?.data.count, 0) | ||
| XCTAssertEqual(blocks.last?.data.count, 99) | ||
| } | ||
| } |
There was a problem hiding this comment.
DataBlockReader is now a core element in the SDK and we only cover happy paths in these tests. Let's add tests for covering pesimisting scenarios:
- reading unrecognized
BlockTypewithDataBlockReader, - reading max and min
BlockSizewithDataBlockReader, - serializing max and min
data.countwithDataBlock.serialize().
There was a problem hiding this comment.
As discussed, we won't be able to test maximum data block sizes (4GB) without redesigning TLV implementation and using generics to mock smaller block size footprint in bytes. Tho, I've added test for 0 bytes data and for large (10MB) data block.
maxep
force-pushed
the
maxep/RUMM-2134/v2-storage
branch
from
28ed87b to
6d98992
Compare
What and why?
Write event to files in TLV format. This change prepares the introduction of event metadata to batches.
How?
All events will be serialised in TLV format using the following byte alignment:
This
block typeis a 2 bytes value describing how to decode the data. In this PR, only code0x00identifying an event is used.The
data sizeis written in 4 bytes so we are more flexible in term of size limit.Note
By using TLV, the format differs from the RFC so we are more resilient to header (metadata) decoding failure.
Batches will be written in
v2folders, this PR does not remove anyv1folder, it will be dealt in another PR.Review checklist
Custom CI job configuration (optional)