-
Notifications
You must be signed in to change notification settings - Fork 13
feat: introduce seatbelt crate #203
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
=========================================
Coverage 100.0% 100.0%
=========================================
Files 105 135 +30
Lines 6756 8150 +1394
=========================================
+ Hits 6756 8150 +1394 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
geeknoid
left a comment
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.
What are your throughs on the fallback and hedging patterns? Do you expect to add those later on?
Currently we only focused for the middlewares that are part of standard resilience pipeline in .NET: Once necessary we will add fallback and hedging next. Currently design will work great for these, actually hedging becomes much simpler and powerful with the design we have now. Hedging requires cloning the input, in .NET because Polly worked with callbacks we had to put some workarounds to get to the original input. Here, it's much more natural as the input is immediately accessible by every middleware layer. |
| //! | ||
| //! # Configuration | ||
| //! | ||
| //! The [`TimeoutLayer`] uses a type state pattern to enforce that all required properties are |
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.
So what's the benefit of using this pattern, rather then just requiring the parameters on the layer function in the first place?
Timeout::layer("timeout", &context, Duration::from_secs(30), |_| io::Error::new(io::ErrorKind::TimedOut, "operation timed out"));That's a lot less typing...
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.
If you look at TimeoutLayer you can notice there are multiple ways on how the required parameters can be configured. This all depends on combination of generics. For example, if output is result-based one can call TimeoutLayer::timeout_error, or just TimeoutLayer::timeout_output if full flexibility is necessary.
Requiring all required parameters to be specified when constructing the layer removes that flexibility or can lead to many layer constructors to be exposed. Same thing applies to circuit-breaker and retries. So rather than deal with many constructors, I embraced the builder approach for everything.
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.
Same thing with RetryLayer, depending on the scenario once can choose from these required methods:
RetryLayer::clone_input_withRetryLayer::clone_inputRetryLayer::recovery_withRetryLayer::recovery
Supporting all these combinations in constructor would be tricky and kinda verbose.
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 definitely see the value of the type state approach in general. My comment is specifically about required parameters. If they're truly required values, then using the type state pattern for those just seems like an extra burden, more keystrokes. And I think it's less intuitive when learning to use an API.
It looks as though I can create a timeout thingy easily. Oh, but I get a (in this case fairly cryptic) compile time error telling me something is wrong. I need to figure out that I must call these other two functions in order to supply parameters.
When it's just there in the constructor as an explicit parameter, it's unambiguous.
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.
We talked with Ralf regarding state pattern and he is generally fine with it. We agreed on doing some simplification regarding the name of the type states.
Regarding the required parameters. Having a constructor to set these and abandon type-state pattern would work if there was only a single way to set these, but this is not the case here. These parameters can be set using multiple methods and these is no simple way to set these using a constructor.
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.
Notes from discussion:
-
PipelineContext->ResilienceContext(or so) - Hide
Set,NotSet - Rename + document typestate variables
- Remove re-exports of
layered -
.build()->(in)to_service(in layered)
Introduces seatbelt, a production-ready resilience middleware crate providing:
Key features:
Example: