Add "Metadata" field to Backends
#304
Conversation
This is intended to make it easier to have custom metadata fields, as HashSet only requires Hash and Eq, while BTreeSet also requires Ord.
The aim for this is to allow HttpPeer to implement Eq, which will allow it to be used as a Metadata field for Backends. This allows users of pingora-load-balancing to directly store an HttpPeer when selecting a backend, for use with the HttpProxy trait implementations.
| // currently this is only provided to allow HttpPeer to be Eq. | ||
| let a: *const dyn Tracing = self.0.deref(); | ||
| let b: *const dyn Tracing = other.0.deref(); | ||
| core::ptr::addr_eq(a, b) |
There was a problem hiding this comment.
This is questionable and could use a review
| fn eq(&self, other: &Self) -> bool { | ||
| // Use destructuring to make sure that we update this for new fields | ||
| let CertKey { certificates, key } = self; | ||
| certificates.eq(&other.certificates) && key.public_eq(&other.key) |
There was a problem hiding this comment.
This is also questionable and could use a review
|
Hey @drcaramelsyrup et. al, I'd be interesting in discussing the viability of this. I believe I've plumbed through the "metadata" (or "bonus data") appropriately, and existing tests pass. However, this ended up being somewhat invasive, so it'd be good to have design feedback here if you all think this is reasonable. If you'd like to see the "end goal" of what this enables, check out this PR in 1.72 tests are failing because I'm using some newer stdlib features, but I can figure out how to work back the MSRV update if this PR overall seems reasonable to you. |
|
Is the meta data part of the unique identifier of the upstream or is it sometime just part of how to connection to the upstream. For example, IP and port identify (define) an unique upstream, while, say, connection timeout is not. The latter is much easier to add. |
|
At the moment in the pingora impl: it's treated as if it was part of the identity (e.g., Hash includes the detail of the data, not just the SocketAddr). At the moment in the river integration: It's the At the moment, that only includes TLS SNI (when relevant), but I do plan to attach additional data like health check routines and such. I could have a separate |
|
This PR implements a feature I find very useful and helps a use case I'm building ontop of Pingora. I have my own "metadata", which is the "age" of a backend, which I utilize in the I imagine others will find this feature to be extremely useful for similar use cases. Thanks for the consideration! |
jamesmunns
changed the title
BackendsBackends
|
One possible design to carry opaque meta data is something like this. The upside is that it requires little to none change in the APIs. The downside is that it is totally opaque so that you can't have, say, two backends with the same socket address but different SNIs. Maybe that is a good enough solution to start with. |
|
@eaufavor I think a type erased approach is possible, generally taking To mind, this has two downsides:
I don't love the idea of having type erased content there, but I would prefer that to having "no metadata at all". |
|
let me take a stab at the idea I proposed |
|
@eaufavor provided me with a patch implementing a dynamic (vs generic) version of this API, which I applied in this commit on the memorysafety fork of I've integrated it into River with memorysafety/river#56, and it appears to be suitable upon basic testing. Closing this PR in favor of the changes proposed in the linked patch (which I assume will land in this repo shortly, and hopefully be released soon after), thanks @eaufavor! |
This is part of an attempt to address #276 (comment), by adding a metadata field that can be any type.
I don't think this is ready to merge, but I'm open to some feedback on the approach, so far.
Open questions/comments:
river, to see if the API allows me to do what I want:HttpHealthCheck->peer_template, in favor of actually having the full information needed perBackend.I'll let you know how it goes!