Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(replays): video item #3149

Merged

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Feb 21, 2024

Serialize processed replay video item back into the envelope instead of putting it on the header.

#skip-changelog

config: &ProjectConfig,
client_ip: Option<IpAddr>,
user_agent: &RawUserAgentInfo<&str>,
) -> ProcessingAction<String> {
match process_replay_event(payload, config, client_ip, user_agent) {
) -> Result<Bytes, Outcome> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmanallen I refactored ProcessingAction to Result<Bytes, Outcome>, let me know if you like it. By using Bytes, which is a lightweight, reference-counted object, we can fold the Keep and Replace cases into one.

Comment on lines +880 to +884
struct VideoEvent<'a> {
replay_event: &'a [u8],
replay_recording: &'a [u8],
replay_video: &'a [u8],
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deserializing this is cheap, because it does not copy any data.

}

ProcessingAction::Replace((replay_event, replay_recording, event.replay_video))
match rmp_serde::to_vec_named(&ReplayVideoEvent {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that we need to serialize here, but it's a known problem that we plan to fix for other item types as well.

If we do not serialize back into the envelope, the data will be lost in PoP Relays.

@jjbayer jjbayer requested a review from cmanallen February 22, 2024 10:37
@jjbayer jjbayer marked this pull request as ready for review February 22, 2024 11:22
@jjbayer jjbayer requested a review from a team as a code owner February 22, 2024 11:22
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, as bad as the useless (but currently necessary) roundtrip through de- & serialization is with the zero copy deserialization is, it should be much less bad.

@cmanallen cmanallen merged commit e6e193c into cmanallen/replays-add-video-envelope-item Feb 22, 2024
21 checks passed
@cmanallen cmanallen deleted the joris/replay-video branch February 22, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants