-
Notifications
You must be signed in to change notification settings - Fork 233
Description
Hi, I hope you will allow me a second chance to argue for the inclusion of #442
About the change
Breaking change - Making Bucket generic (
Bucket<B>) is a significant API break that affects all downstream users, requiring them to update their code even if they don't need pluggable backends.
It is indisputably true that this is a breaking change and needs to be applied across a semver bump.
I grant that the introduction of a generic parameter to Bucket is disruptive. There are 2 ways to counter that, either one of which I would be happy to change to.
- (Mitigates only) Add a default value for the generic type. Then downstream users can continue to refer to plain
Bucketunless they have actually configured a different (custom) backend. - (Solves completely) The backend could be made into a
Box<dyn Service<_>>and then the generic parameter disappears completely. This comes at the cost of a heap allocation (only once per bucket) and vtable lookup, but neither is that expensive compared to what else is going on.
Complexity vs benefit tradeoff - This adds substantial complexity (generic parameters propagating throughout the codebase) and new dependencies (tower-service, http-body, http-body-util) for a use case that most users won't need.
Maintenance burden - The added abstraction layer makes the codebase harder to maintain and reason about.
To both of these points, I would respond that coomplexity and maintainability is actually improved:
- Better separation of layers: there used to be a couple of fields in the
Bucketstruct that pertained only to the reqwest backend. Now, each backend has its own type that doesn't pollute the Bucket type. Indeed the only conditional compilation directives left inbucket.rsaren't really for backend selection but only for tokio/async-std/blocking selection. - Unlocks better testing: Not in this pull request (which is big enough already) but for subsequent work: many of the tests currently do not execute by default because they reach out to real buckets or require a local S3 server. For sure that kind of integration test is useful, but a replaceable backend enables a bunch of more practical unit tests to be written, by creating a mock backend.
- Featured currently triplicated can be factored out: Again not in this pull request but for subsequent work: there are 2 features that currently are coded 3 times, once for each backend: the retry logic and the fail-on-err logic. Both features could use some work (for example the retry logic is configured with globals, something which could become a problem if 2 independent users of rust-s3 exist in the same binary but want different settings) and both features are ideal candidates for implementing singly as a
tower::Layerand then applicable to any backend (including custom ones).
On the additional dependencies: FWIW all 3 are already present in the tree if reqwest is configured. Admittedly that's not true if surf is configured, but I'd mention that tower-service and http-body are both deliberately tiny trait-only crates, and as for http-body-util it could probably be removed if that's a sticking point.
About the utility of a replaceable backend
I think that a replaceable backend may be more useful and less niche than it appears.
Take a look first at how at least one person decided that a competing crate with no backend at all was a good idea: rusty-s3. They call it "Sans-IO". It looks like it just prepares the HTTP request for you, making the user furnish their own backend and do the work of pumping it through. That crate's actual S3 feature support seems to be much less well developed than yours, which is why I didn't consider it. I cite it only as the intended usage pattern is interesting.
As for reasons why a pluggable backend can be useful, there are a myriad things that a user might want that the stock backends won't provide which can be added either as alternate backends or tower layers: client request metrics and tracing, backend subsetting and load balancing, backend connection health diagnostics, deadline propagation, custom TLS parameters, health status propagation, prewarming client connections, and more. Using this change, I can already nearly test-fit warm_channels as a custom backend to provide a lot of this.
Thank you for your consideration