Repository navigation
grpc/client: support custom call options - #2878
Conversation
| //! | ||
| //! [`Attributes`](crate::attributes::Attributes) is a persistent, immutable | ||
| //! map shared between components: adding to it produces a new map. | ||
| //! [`CallAttributes`] instead it is mutated in place. |
There was a problem hiding this comment.
Maybe we can come up with better names that describe what the different attribute types are/do, instead of where they're used?
Options?
Attributes -> ImmutableAttributes
CallAttributes -> Attributes
or
Attributes -> AttributeChain
CallAttributes -> AttributeMap
Or... is it feasible to merge these two types by adding a way to layer these new Attributes?
There was a problem hiding this comment.
How about using the prefix "Persistent" for the existing attributes and no prefix for the new ones? Some options:
- PersistentAttributes and Attributes
- PersistentTypeMap and TypeMap
There was a problem hiding this comment.
FWIW 'persistant' tends to make me think of things that live past the immediate scope of use - written to disk, or whatever. Maybe FixedAttributes? Though I like Doug's layering idea, too.
There was a problem hiding this comment.
"Persistent" is a technical term for data structures that preserve their previous versions when modified. They are immutable and rely on structural sharing to perform updates efficiently without copying the entire structure: https://en.wikipedia.org/wiki/Persistent_data_structure
There was a problem hiding this comment.
Sure, but the idea of persistance is any state that outlasts the process that creates it. It could be confusing, even if technically precise. I don't really have a horse in the race, though - just a thought for your consideration.
There was a problem hiding this comment.
Is it feasible to merge these two types by adding a way to layer these new Attributes?
No, I don't think merging them is beneficial. The two types are optimized for opposite workloads:
Attributesmaximizes structural sharing to save memory. They're attached to every endpoint (there can be thousands) and are repeatedly extended by LB policies.CallAttributesis optimized for fast insertion and lookup on the RPC path.
Structural sharing requires values to be Sync. We should avoid imposing that on per-call data so that we can introduce per-call arenas later. Merging the types would also hurt both sides:
- RPC path: extra atomic refcount operations and extra pointer indirection.
- Endpoint
Attributes: larger per-entry size.
Other gRPC implementations also use separate data structures for the RPC and non-RPC paths:
- Java:
AttributesandCallOptions. - Go:
attributes.Attributesandcontext.Context. - C++:
ChannelArgsand call attributes (ServiceConfigCallData::CallAttributeInterface, read viaClientChannelLbCallState::GetCallAttribute<T>()).
I've renamed Attributes to ImmutableAttributes and CallAttributes to Attributes. I avoided AttributeChain since it leaks an implementation detail that may not be guaranteed in the future (such as if we transition to a persistent tree structure). This implementation detail is also not relevant to end users.
| let result = &state.picker.pick(&headers); | ||
| let result = state | ||
| .picker | ||
| .pick(PickOptions::new(&headers, options.attributes_mut())); |
There was a problem hiding this comment.
Given that this is in a loop, what happens if the pick() both modifies the call attributes and returns a Queue? What is the expected behavior there? Is it fine for the call attributes to aggregate writes to it while in a queuing state?
There was a problem hiding this comment.
Yes, since pickers can mutate the call attributes, mutations made by a queueing picker will get carried over to the next pick call.
I can think of a few solutions to this problem:
- "Trust" the queueing picker implementations to be idempotent.
- Pass
&CallAttributesinstead of&mut CallAttributes: If a picker needs to modify theCallAttributes, it must clone the existing ones first, modify the copy and delegate. To propagate changes from pickers to interceptors (C++ does this for session affinity), we will need to use interior mutability. Note that sinceCallAttributesis!Sync,&CallAttributesis!Send. However, this should not be a problem since thepickmethod is notasync. Config selectors will get mutable references. - Use the existing persistent
Attributesstruct. The picker takes ownership and returns attributes. The channel can simply discard the attributes returned by a queuing picker. This incurs the overhead of pointer chasing, ref-counting, and slightly higher memory overhead than a mutable, vector-backed data structure. - Have the channel clone the attributes before calling
pick. Choose which attributes to keep depending on whether the RPC is queued. This incurs a clone for each RPC.
For comparison, this is how other languages handle this:
- Java's
CallOptionsare immutable; they copy the options list to modify it. This is similar to option 3. - Go uses Context.WithValue to wrap existing contexts. This is similar to option 3.
- C++ has mutable call attributes. This is similar to option 1.
@dfawley I wanted to get your thoughts on this.
There was a problem hiding this comment.
What about retry? Are we keeping an unmodified copy for retry in case we have to re-pick & re-run interceptors? How does C++ make sure SSA or call-attribute-modifying-interceptors work correctly with retry? (Do they make a copy?)
There was a problem hiding this comment.
What about retry? Are we keeping an unmodified copy for retry in case we have to re-pick & re-run interceptors?
Presently, retry is modeled as an interceptor and invoke takes ownership of the CallOptions. So the CallOptions must be cloned:
grpc-rust/grpc/src/client/interceptor.rs
Line 667 in 988d4d7
This will result in the call attributes being cloned as well. C++ passes a mutable reference of the call attributes to filters, so it doesn't require copying them.
How does C++ make sure SSA or call-attribute-modifying-interceptors work correctly with retry?
In C++, the order execution is: config selector → SSA (and other xDS filters) → retry → one LB pick call per attempt.
Changes made before retry stay put. The SSA cluster override and the cookie address list are written once by the config selector and SSA filter, before the retry layer. Every attempt's pick sees the same values.
The LB policy writes to the shared attribute on each attempt. xds_override_host's picker overwrites the actual_address_list_ on each attempt based on whether the SSA override was applied or not.
There was a problem hiding this comment.
I made the attributes ref in PickOptions immutable, so pickers can't mutate them. Only the config selector and interceptors can mutate the call attributes now.
dfawley
left a comment
There was a problem hiding this comment.
Maybe we avoid the churn and keep Attributes as-is, and call these new things CallOptions since....do we have any other intended use for them? Let's discuss offline.
This reverts commit 37aab01.
|
Based on offline discussions, I've moved the accessors to the |
## Motivation Implementation of [XDS Cluster Manager](https://github.com/grpc/proposal/blob/master/A31-xds-timeout-support-and-config-selector.md) ## Solution Leverages ChildManager to manage child policies. Carries functionality to parse ClusterManager-specific configuration, do the resolver fanout and picker assemblage and handles the per-rpc routing and enforcement of drops on missing/unknown attributes/clusters. ### Notes * Currently this PR relies on two other PRs for refactors that make this job a lot easier: #2878, #2887. * Deferring removed-child grace period. See #2890. * Config Selector being landed is required for correctness around removal of clusters. --------- Co-authored-by: Arjan Bal <arjansbal@google.com>
Adds a type-keyed map in
CallOptionsfor passing arbitrary data between interceptors, the LB picker, and the upcoming config selector. It stores at most one value per type, keyed byTypeId, and is similar tohttp::Extensions.Two properties are driven by the call path:
Send— interceptors are async, so the map must be holdable across.awaitpoints and movable between threads.Clone— retries need an independent copy, soCloneis a deep copy and stored types are required to beClone.Unlike
http::Extensions, stored values are not required to beSync. This is mainly because the bumpalo::Bump type is!Sync. So the bound can't be satisfied if an arena is introduced later.Why not an arena?
A bumpalo-backed implementation was prototyped and benchmarked against the
Vecimplementation for 5–15 attributes. The differences were small and inconsistent at those sizes, and the arena cost a new dependency plus substantially moreunsafe, forged'staticlifetimes on arena-allocated boxes,MaybeUninitslot management, and hand-written drop ordering. Not worth it, so the plainVecimplementation is chosen.Benchmarks
Lifecycle Benchmark (Creation, Insertions, Tear-down)
Lookup Benchmark (Retrieval by Type)