xds: explicitly set request hash key for the ring hash LB policy #11881
Conversation
shivaspeaks
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
| if (headerValue != null) { | ||
| requestHash = hashFunc.hashAsciiString(headerValue); | ||
| } else { | ||
| requestHash = random.nextLong(); |
There was a problem hiding this comment.
This is not thread-safe. See io.grpc.xds.ThreadSafeRandom.
| @@ -334,23 +339,30 @@ public static EquivalentAddressGroup stripAttrs(EquivalentAddressGroup eag) { | |||
| } | |||
|
|
|||
| private static final class RingHashPicker extends SubchannelPicker { | |||
| private final Random random = new Random(); | |||
There was a problem hiding this comment.
We probably want to share this object across pickers and even inject it when creating RingHashLB for tests. That way we don't have to deal with flakes.
| private final SynchronizationContext syncContext; | ||
| private final List<RingEntry> ring; | ||
| // Avoid synchronization between pickSubchannel and subchannel's connectivity state change, | ||
| // freeze picker's view of subchannel's connectivity state. | ||
| // TODO(chengyuanzhang): can be more performance-friendly with | ||
| // IdentityHashMap<Subchannel, ConnectivityStateInfo> and RingEntry contains Subchannel. | ||
| private final Map<Endpoint, SubchannelView> pickableSubchannels; // read-only | ||
| private final String requestHashHeader; | ||
| private boolean hasEndpointInConnectingState = false; |
| return PickResult.withError(RPC_HASH_NOT_FOUND); | ||
| // Determine request hash. | ||
| boolean usingRandomHash = false; | ||
| Long requestHash; |
There was a problem hiding this comment.
Let's not box and think about null in these new cases that don't need it. We should make this long and deal with the null in the only case that needs it. We should also change getTargetIndex() be be passed a long (since it assumes it is non-null already).
| private final SynchronizationContext syncContext; | ||
| private final List<RingEntry> ring; | ||
| // Avoid synchronization between pickSubchannel and subchannel's connectivity state change, | ||
| // freeze picker's view of subchannel's connectivity state. | ||
| // TODO(chengyuanzhang): can be more performance-friendly with | ||
| // IdentityHashMap<Subchannel, ConnectivityStateInfo> and RingEntry contains Subchannel. | ||
| private final Map<Endpoint, SubchannelView> pickableSubchannels; // read-only | ||
| private final String requestHashHeader; |
There was a problem hiding this comment.
I think we want to move the construction of Metadata.Key<String> earlier and store the object here. It'd be null when the hash header is empty string. We should also store the key in RingHashLoadBalancer.requestHashHeader. It's up to you whether we store it in RingHashConfig.
Implements gRFC A76: explicitly setting the request hash key for the ring hash LB policy
GRPC_EXPERIMENTAL_RING_HASH_SET_REQUEST_HASH_KEYenvironment variable until API stabilized.Tested:
ring_hash_experimentaland a customrequest_hash_header(with NO associated value in the metadata headers) which generates a random hash for each request to the ring hash LB. Verified picks/RPCs are split evenly/uniformly across all backends.CC: @shivaspeaks