Skip to content

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

Merged
teh-cmc merged 8 commits intomainfrom
jleibs/explicit_recording_ids
May 15, 2023
Merged

Make every RecordingId typed and preclude the existence of 'Defaults'#2110
teh-cmc merged 8 commits intomainfrom
jleibs/explicit_recording_ids

Conversation

@jleibs
Copy link
Copy Markdown
Contributor

@jleibs jleibs commented May 12, 2023 •
edited by github-actions bot
Loading

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
teh-cmc
teh-cmc approved these changes May 15, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

@teh-cmc
Copy link
Copy Markdown
Contributor

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 teh-cmc mentioned this pull request May 15, 2023
Merged
4 tasks
@teh-cmc
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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.

jleibs
jleibs commented May 15, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor 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

Reviewers

1 more reviewer

@teh-cmc teh-cmc teh-cmc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🚜 refactor Change the code, not the functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jleibs @teh-cmc