-
Notifications
You must be signed in to change notification settings - Fork 639
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
Refactor the gRPC enumerators #3998
Refactor the gRPC enumerators #3998
Conversation
DB-392 Refactor the grpc enumerators
So that they aren't coupled to grpc and can be used by the connectors plugin hosting mechanism in the node |
48b5ea9
to
f25b832
Compare
_expiryStrategy = expiryStrategy; | ||
_subscriptionId = Guid.NewGuid(); | ||
_bus = bus; | ||
_bus = bus ?? throw new ArgumentNullException(nameof(bus)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could be persuaded, but i think i'd be tempted to leave these tidyups for another PR, so that this one can be as minimal and obviously correct as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I will update this.
|
||
private void Fail(Exception exception) { | ||
Interlocked.Exchange(ref _subscriptionStarted, 1); | ||
_channel.Writer.TryComplete(exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this closes the channel with an exception but most of that places that used to call this now close the channel cleanly or dont close it at all, is that an intentional change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I am writing an item to the channel (ReadResponse) rather than exception (grpc) so that's why I just close the channel without an exception. While enumerating through the channel in Stream.Read.cs I used the grpc exception based on the case.
In case of UnknownResponseException, I have closed the channel with exception.
|
||
} | ||
|
||
public class ResolvedEventWrapper: ReadResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be nice if these were nested in the abstract base class, its a pattern we often do with the message classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
9ee31d7
to
969a539
Compare
969a539
to
bccea05
Compare
Co-Authored-By: shaan1337 <sniper111@gmail.com>
Co-Authored-By: shaan1337 <sniper111@gmail.com>
…mespace Co-Authored-By: shaan1337 <sniper111@gmail.com>
Co-Authored-By: shaan1337 <sniper111@gmail.com>
1f36dac
to
9162585
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! 👍
Added: Refactor gRPC enumerators
Fixes https://linear.app/eventstore/issue/DB-392/refactor-the-grpc-enumerators
Decouple all the enumerators from gRPC so they can work directly with connectors.