Data track schema metadata#1159
Conversation
ChangesetThe following package versions will be affected by this PR:
|
Adds request ID fields
|
For local testing:
|
ladvoc
requested review from
1egoman,
MaxHeimbrock,
alan-george-lk,
lukasIO,
stephen-derosa and
xianshijing-lk
as code owners
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally makes sense to me!
I think DataTrackSchemaId being a non primative value is key here to future interface evolution (like adding a generic type with a default which when set explictly can be used to assert the type of payloads later on).
| const DATA_BLOB_REQUEST_TIMEOUT: Duration = Duration::from_secs(5); | ||
|
|
||
| /// Stores an arbitrary blob of data on the server, keyed by `key`. | ||
| async fn store_data_blob(&self, key: proto::DataBlobKey, contents: Bytes) -> EngineResult<()> { |
There was a problem hiding this comment.
question: Should this be RoomResult<()>, not EngineResult<()>? There's a few other instances of this in the file as well.
| /// nor validated against its [encoding](DataTrackSchemaId::encoding), so | ||
| /// the caller is responsible for ensuring it is well-formed. | ||
| /// | ||
| pub async fn define_schema(&self, id: DataTrackSchemaId, definition: String) -> RoomResult<()> { |
There was a problem hiding this comment.
thought: Are you confident that schemas will / should always be text? Or should definition: String be some sort of bytes object, like Vec<u8> or bytes::Bytes (or maybe impl Into<bytes::Bytes>)?
There was a problem hiding this comment.
Yes, while the wire format (frame content) will often be binary, I can't think of any case where the schema definition language would be binary. @stephen-derosa, wdyt?
| let (pub_room, _) = rooms.pop().unwrap(); | ||
| let (_, mut sub_room_event_rx) = rooms.pop().unwrap(); | ||
|
|
||
| let schema_id = DataTrackSchemaId::new("my_schema", DataTrackSchemaEncoding::JsonSchema); |
There was a problem hiding this comment.
question: Is this DataTrackSchemaId::new(...) expected to be a typical pattern, maybe in the case that you know that a schema already exists on the SFU end? Or is the idea that in practice, a user would always call LocalParticipant::deine_schema and this is just being done this way for testing?
stephen-derosa
left a comment
stephen-derosa
left a comment
There was a problem hiding this comment.
in general looks good, need to give it a deeper dive
| optional SubscribeDataTrackError error = 1; | ||
| } | ||
|
|
||
| // MARK: - Schemas |
There was a problem hiding this comment.
FMU, what is this // MARK: notation?
There was a problem hiding this comment.
This is recognized by most editors (see the minimap and navigator in Cursor/Vscode).
| } | ||
|
|
||
| #[test] | ||
| fn test_frame_encoding_mapping() { |
There was a problem hiding this comment.
test_empty_frame_encoding ?
| /// | ||
| /// Called by a publisher to make a schema available to subscribers, who can | ||
| /// later look up its definition via [`get_schema`](Self::get_schema). Define a | ||
| /// schema before publishing any data track that references it, so that |
There was a problem hiding this comment.
what is the behavior if a data track with a schema is published before the schema itself is published?
There was a problem hiding this comment.
The track will be published normally and carry the schema ID. If you care about retrieving it on the subscriber end, it is recommended to store the schema before publishing the track to ensure it is available right away, but if you don't need the actual definition for you application, you can simply use schema ID as an identifier.
3 participants
Adds support for associating schema metadata with a published data track and storing/retrieving schema definitions.
Schema storage is built on-top of data blobs, a general purpose mechanism for storing large (in the order of KBs), arbitrary data blobs in a room:
Protocol additions for schema metadata: