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

Implement RFC 574 #195

Merged
merged 2 commits into from
Feb 16, 2022
Merged

Implement RFC 574 #195

merged 2 commits into from
Feb 16, 2022

Conversation

thefringeninja
Copy link
Contributor

@thefringeninja thefringeninja commented Feb 10, 2022

Added: Set grpc.max_receive_message_length to 17MB
Fixed: Force Rediscovery Only when Lost Connection or Unknown error

EventStore allows events of up to 16mb to be written internally. While you can't write events this large through gRPC, you could do so through the TCP client, or through projections. To allow the gRPC clients to read any event that EventStoreDB was able to write, we want to hardcode the max receive message length for all gRPC clients to 17mb.

Some cases where redicovery should have been forced were not handled, are handled now.

Fixes #193

@thefringeninja thefringeninja self-assigned this Feb 10, 2022
@hayley-jean hayley-jean marked this pull request as ready for review February 10, 2022 12:28
@thefringeninja thefringeninja marked this pull request as draft February 10, 2022 12:31
@thefringeninja thefringeninja force-pushed the rfc-574 branch 3 times, most recently from a333c54 to cdcfeec Compare February 10, 2022 13:53
@thefringeninja thefringeninja marked this pull request as ready for review February 10, 2022 13:53
pvanbuijtene
pvanbuijtene previously approved these changes Feb 14, 2022
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

  • the size restrictions look good
  • adding a ReconnectionRequired type looks good

but im suspicious about

  • rediscovering on 'aborted' status
  • the number of InnerExceptions to dig through - we might need that extra one that was there before because of the typedexceptioninterceptor

oskardudycz
oskardudycz previously approved these changes Feb 15, 2022
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

The 'aborted' thing is resolved now (rfc is adjusted or in the processof)

I think still need to address the number of InnerExceptions we are looking in to find the RpcException, because the TypedExceptionHandler will wrap it in an InvalidOperationException. If I'm right then UNAVAILABLE will not trigger rediscovery

oskardudycz
oskardudycz previously approved these changes Feb 16, 2022
@timothycoleman timothycoleman added the breaking This PR contains a breaking change label Feb 16, 2022
@timothycoleman
Copy link
Contributor

For documentation we should explain that we aren't wrapping in InvalidOperation exception any more

Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

nice 👍

Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

only couple of tests to update

timothycoleman
timothycoleman previously approved these changes Feb 16, 2022
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@oskardudycz oskardudycz left a comment

Choose a reason for hiding this comment

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

Wait a moment, why are we dropping InvalidOperationException support? It's also not aligned with a recent @hayley-jean suggestion: https://github.com/EventStore/architecture-and-planning/pull/106

- set grpc.max_receive_message_length to 17MB
- force rediscovery only when lost connection or unknown error
@thefringeninja thefringeninja merged commit 24fa50c into EventStore:master Feb 16, 2022
@thefringeninja thefringeninja deleted the rfc-574 branch February 16, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This PR contains a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase max receive message length to match the max event size in EventStoreDB
4 participants