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

feat: add an topic event enumerable #575

Merged
merged 7 commits into from
Sep 23, 2024
Merged

feat: add an topic event enumerable #575

merged 7 commits into from
Sep 23, 2024

Conversation

malandis
Copy link
Contributor

@malandis malandis commented Sep 19, 2024

Introduces an interface ITopicEvent which covers both normal
messages (TopicMessage) and system events (TopicSystemEvent). We
refactor the internals of the stream to be over ITopicEvents, so
that we can filter to just TopicMessage (as is implemented
currently) or pass through all events in an event enumerable.

To enumerate over all events in the stream, call GetAllEventsAsyncEnumerable,
or WithCancellationForAllEvents.

This largely parallels the existing enumerator over messages only. See the
integration tests for an example usage.

Introduces an interface `ITopicEvent` which covers both system
events (`TopicEvent`) and normal messages (`TopicMessage`). We
refactor the internals of the stream to be over `ITopicEvent`s, so
that we can filter to just `TopicMessage` (as is implemented
currently) or pass through all events in an event enumerable (to come
in a future commit).
@malandis malandis marked this pull request as draft September 19, 2024 23:39
@malandis
Copy link
Contributor Author

malandis commented Sep 19, 2024

Opening for early review for opinions on naming and organization. Currently the subscription response object has a GetAsyncEnumerable method which allows users to consume TopicMessage instances -- text, binary, or error. We will add a new method GetEventAsyncEnumerable which will allow users to consume TopicMessage as well as heartbeats and discontinuities.

To organize this we've introduced an interface ITopicEvent which both TopicMessage and TopicSystemEvent will implement. The latter covers the system events heartbeat and discontinuity.

@nand4011
Copy link
Contributor

'event' feels like 'a thing that happened' to me. I like it for TopicSystemEvent, but I'm not as sure about ITopicEvent also representing normal messages. Maybe ITopicItem? I don't have too strong an opinion on it though.
The same thing with GetEventAsyncEnumerable. By the name alone I would assume it would be for exclusively system events, but the docs would clear that up. GetUnabridgedAsyncEnumerable could work, but might be too much of a mouthful.

@cprice404
Copy link
Contributor

'event' feels like 'a thing that happened' to me. I like it for TopicSystemEvent, but I'm not as sure about ITopicEvent also representing normal messages. Maybe ITopicItem? I don't have too strong an opinion on it though. The same thing with GetEventAsyncEnumerable. By the name alone I would assume it would be for exclusively system events, but the docs would clear that up. GetUnabridgedAsyncEnumerable could work, but might be too much of a mouthful.

There was a thread in slack when we did this work for golang, and Event is the name that most people settled on, so as far as the public API for this goes I think we should be trying to mirror (semantically) what we did in go. I think it probably makes sense for the parent type / interface to include the word Event even though I do agree there is a bit of ambiguity for normal messages.

Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

i think i'm good with the names so far.

might be good to get @anitarua to weigh in since she did the prev two.

@anitarua
Copy link
Contributor

Yeah, I think the names are fine here, ITopicEvent implemented by TopicMessage and TopicSystemEvent.

In the Go SDK, TopicEvent was implemented by TopicItem (string or byte messages), TopicHeartbeat, and TopicDiscontinuity.

Previously the subscription stream pruned heartbeats and
discontinuities before reaching the subscription response
subscription. Since we will consume heartbeats and discontinuities
in the event enumerable, we pass these through and ignore in the
message enumerator.
@malandis
Copy link
Contributor Author

Given the consensus, I've left the names as is. As Anita pointed out, there's a difference in naming vs the Golang SDK.

In the protos we have SubscriptionItem and TopicItem. SubscriptionItem now matches ITopicEvent and covers text, binary, heartbeat, and discontinuities. TopicItem in the protos has been called TopicMessage in .NET. We'll leave the latter as is for backwards compatibility.

Adds an async enumerator over all events, not just messages. This
largely parallels the existing enumerator over messages only. See the
integration tests for an example usage. Call
`WithCancellationForAllEvents` to get the enumerator.
@malandis malandis marked this pull request as ready for review September 23, 2024 19:56
nand4011
nand4011 previously approved these changes Sep 23, 2024
tests/Integration/Momento.Sdk.Tests/Topics/TopicTest.cs Outdated Show resolved Hide resolved
@malandis malandis merged commit e00c218 into main Sep 23, 2024
8 checks passed
@malandis malandis deleted the feat/topic-event-api branch September 23, 2024 21:45
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