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

Make every RecordingId typed and preclude the existence of 'Defaults' #2110

Merged
merged 8 commits into from
May 15, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented May 12, 2023

What

After talking over the confusing edge cases with @teh-cmc around "where/how do we end up with unknown recording types and unknown recording-ids" I decided to go ahead and force everything to be explicit. This simplifies a number of edge-cases related to blueprint-recordings.

  • Every RecordingId now includes the RecordingType. You cannot create a RecordingId without specifying the type. There is no default for RecordingId.
  • Every LogDb also knows it's RecordingId. You cannot create a LogDb without knowing it's RecordingId. This adds a bit more pain to the process of creating a LogDb, but again means you don't have to worry about having a LogDb instance and not knowing it's id / type.
  • Every LogMsg must have a RecordingId. This meant adding a RecordingId to the Goodbye message.
  • The selected_recording_id instead is now Optional. You can only select a valid recording.

Overall, this makes some things a bit more verbose, but generally seems to improve the clarity by reducing the number of places we end up doing work with unexpected ephemeral log-dbs for an unknown recording of an unknown type.

Checklist

PR Build Summary: https://build.rerun.io/pr/2110

@jleibs jleibs added the 🚜 refactor Change the code, not the functionality label May 12, 2023
@jleibs jleibs marked this pull request as ready for review May 12, 2023 20:49
@teh-cmc teh-cmc self-requested a review May 15, 2023 07:07
Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Looks fantastic! My only gripe is the fact that the transport implementation has a say in when and what kind of app messages get sent (a problem that predates this PR), which now results in a bunch of awkward piping of the RecordingId.

I think we can fix both problems with very little effort by putting RecordingStream in charge of the Goodbye message?

See inline comments.

crates/rerun/src/run.rs Show resolved Hide resolved
crates/re_sdk_comms/src/buffered_client.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/app.rs Show resolved Hide resolved
crates/re_viewer/src/app.rs Outdated Show resolved Hide resolved
@teh-cmc
Copy link
Member

teh-cmc commented May 15, 2023

Addressed my own comments so I can build on top of this for #2061.

I cannot put you as reviewer because you're the author @jleibs, but I'm implicitly doing so :)

@teh-cmc
Copy link
Member

teh-cmc commented May 15, 2023

Fixing the broken test :slow_beach_ball:

@teh-cmc teh-cmc force-pushed the jleibs/explicit_recording_ids branch from 46761b9 to 35782c5 Compare May 15, 2023 13:08
@teh-cmc
Copy link
Member

teh-cmc commented May 15, 2023

This now sends a Goodbye message anytime you call disconnect(), or if you drop the stream altogether.

While I like it better than what we had before, I'm certainly not satisfied with this... in the future I wonder if we shouldn't have a mirrored relationship between OpenedSink (roughly what we call BeginRecordingMsg today) and ClosedSink (i.e. Goodbye) and always send them as we swap a new sink in and out, respectively.
Not only the symmetry would get rid of the fact that the TCP sink currently play by its own rules, it would also open some opportunities UI-wise I think.

Copy link
Member Author

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Nice cleanup; sending from the Stream is a much better solution. Thanks!

@teh-cmc teh-cmc merged commit 8f27fb8 into main May 15, 2023
@teh-cmc teh-cmc deleted the jleibs/explicit_recording_ids branch May 15, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants