-
Notifications
You must be signed in to change notification settings - Fork 37
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
Implement RFC 032 gRPC Client Deadlines #194
Conversation
f2e2f75
to
abf824e
Compare
abf824e
to
ae175a6
Compare
@@ -255,7 +258,13 @@ await foreach (var response in call.ResponseStream.ReadAllAsync(_cancellationTok | |||
|
|||
try { | |||
writeResult.TrySetResult(response.ToWriteResult()); | |||
} catch (Exception ex) when (ex is not RpcException) { | |||
} catch (RpcException ex) when (ex.StatusCode is not StatusCode.NotFound or |
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.
QUESTION: Why was this additional condition added for RpcException
here?
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.
'Normal' errors on batch append will be sent inside the proto itself and will not be handled by grpc.net. These status codes represent grpc errors that you might see (for example NotFound if the endpoint is not there or Unavailable if it's not started yet) and then you will need to close the channel.
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 think a comment would be in order to explain why its these specific statuses
... although DeadLineExceeded should probably be in this list shouldn't it, otherwise a timeout would cause the batchappender to terminate
... in fact, isn't DeadlineExceeded the only possible RpcException we could get here?
src/EventStore.Client.ProjectionManagement/EventStoreProjectionManagementClient.Statistics.cs
Outdated
Show resolved
Hide resolved
test/EventStore.Client.PersistentSubscriptions.Tests/Bugs/Issue_1125.cs
Outdated
Show resolved
Hide resolved
...ent.PersistentSubscriptions.Tests/SubscriptionToAll/when_writing_and_filtering_out_events.cs
Outdated
Show resolved
Hide resolved
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.
looks good, minor comments in line, want to lookg at append timeouts a bit more too
49db224
to
9032b3b
Compare
i pushed a branch then there is just the just the configureOptions convo left and the RpcException handling in batchappender |
@timothycoleman, @thefringeninja, for the future, it'd be good to not squash changes into a single commit on such a big PR before getting final reviews, as it's hard to see what has changed or not and don't have to go through the whole PR doing the review again. |
cfbd6ef
to
d34de6d
Compare
d34de6d
to
cf2c284
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.
looks good 👍
Implement RFC 032 gRPC Client Deadlines
Changed: Default deadlines for reading operations set to
Inifinity
Changed: Deadlines for all other operations defaults to
10s
Changed: Removed
Timeout
fromEventStoreOperationOptions
and moved it to an explicitdeadline
parameter on all operations except for subscriptions. Consequently,configureOperationOptions
callback has been removed for most operations.This PR configures default deadlines for each operation according to the following table:
defaultDeadline
deadline
infinity
infinity
10 seconds
10 seconds
10 seconds
This aligns with the other clients.
Fixes #184