Skip to content

rust: Add SinkId for identifying sinks #290

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

Merged
merged 3 commits into from
Mar 17, 2025
Merged

Conversation

gasmith
Copy link
Contributor

@gasmith gasmith commented Mar 14, 2025

Changelog

  • rust: Add SinkId and new mandatory Sink::id() trait method.

Description

We've been using Arc pointers to compare log sinks, which is… fine. But it'd be a bit nicer if we just had a SinkId to use, like we have a ChannelId. That would give us a natural way of indexing sinks, which will be useful for both sink deregistration as well as subscription management.

Copy link

linear bot commented Mar 14, 2025

@gasmith gasmith self-assigned this Mar 14, 2025
@gasmith gasmith requested review from eloff and bryfox and removed request for eloff March 14, 2025 16:07
@gasmith gasmith marked this pull request as ready for review March 14, 2025 16:07
@gasmith gasmith requested a review from eloff March 14, 2025 16:07
Copy link
Contributor

@bryfox bryfox left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@jtbandes
Copy link
Member

Where is the actual comparison happening (and generally who is going to be using this)?

@gasmith
Copy link
Contributor Author

gasmith commented Mar 14, 2025

Where is the actual comparison happening (and generally who is going to be using this)?

It's primarily for internal use. Currently, Context maintains a LogSinkSet on the basis of arc pointer equality (ref). I'm working to replace that structure with a more sophisticated subscription manager (#281), and I'd much rather be able to index sinks by an integer identifier (ref).

Copy link
Contributor

@eloff eloff left a comment

Choose a reason for hiding this comment

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

This seems fine, it should make it easier to work with hashmaps

@gasmith gasmith force-pushed the gasmith/fg-10743-subscr branch from 9bd1af0 to 5a973fa Compare March 17, 2025 16:30
@gasmith gasmith force-pushed the gasmith/fg-10746-sink-id branch from 7e07e95 to 4fb5fa9 Compare March 17, 2025 18:03
@gasmith gasmith changed the base branch from gasmith/fg-10743-subscr to main March 17, 2025 18:03
@gasmith gasmith merged commit 879a0ed into main Mar 17, 2025
38 checks passed
@gasmith gasmith deleted the gasmith/fg-10746-sink-id branch March 17, 2025 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants