feat(grpc): Add attributes API #2497
Conversation
There was a problem hiding this comment.
Why doesn't something like the im crate satisfy our needs ? AVL trees seem relatively low level for us to bother implementing if an implementation already exists.
There was a problem hiding this comment.
The decision to implement an AVL tree was made to avoid pulling in a new dependency. Since gRPC is used across many projects, any library we add increases the binary size for all downstream users. While the im crate might be more performant due to its use of B-trees, this struct is only used for control plane operations. In my opinion, the performance gain doesn't justify the additional dependency.
I’m open to changing this if we decide that taking on the dependency is acceptable.
There was a problem hiding this comment.
It is acceptable to me 1.6k . Although, I'd probably prefer rpds : https://crates.io/crates/rpds , since it's more actively maintained with simiar-ish star count.
I'll defer the decision to @dfawley , if depending on a create for data structures is fine.
| /// Stored types must implement `Any + Send + Sync + Eq + Ord + Debug`. | ||
| #[derive(Clone, Default, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub struct Attributes { | ||
| map: Avl<TypeId, AttributeValue>, |
There was a problem hiding this comment.
Okay for now.
But maybe we don't want tie ourselves to an implementation.
Can we abstract operations to a trait instead of concrete AVL to decouple things?
There was a problem hiding this comment.
I don't believe any public methods on the Attributes type expose its internal data structure. Could you clarify the benefit of introducing a trait here?
Introducing an interface for a single implementation feels like unnecessary indirection.
grpc/src/attributes/mod.rs
Outdated
| mod avl; | ||
|
|
||
| trait AttributeTrait: Any + Send + Sync + Debug { | ||
| fn as_any(&self) -> &dyn Any; |
|
|
||
| mod avl; | ||
|
|
||
| trait AttributeTrait: Any + Send + Sync + Debug { |
There was a problem hiding this comment.
comments for the trait and the methods? It's particularly difficult to understand the need and requirements for ordering ?
There was a problem hiding this comment.
NVM. The implementation makes it clear but still worth documenting
There was a problem hiding this comment.
Added a rustdoc mentioning that it's required to support value based equality checks instead of pointer comparisions.
|
Added initial set of comments. I'll need some time to learn and review the AVL tree. (or maybe I'll just ask gemini to compare the c++ and rust impl ). |
3 participants
Attributes Implementation
This change introduces an implementation of the
Attributestype—an immutable map designed for storing arbitrary data.To ensure efficient immutability, the map is backed by a persistent AVL tree, with an implementation based on gRPC C++'s
avl.h. Much like http::Extensions in the Rust ecosystem, these attributes are keyed by the value'sTypeId.Attributeswill facilitate data transfer between various control plane components, including resolvers, LB policies, credentials, and the channel.