-
Notifications
You must be signed in to change notification settings - Fork 4.1k
rpc: unify InternalLocalClient optimization with regular request paths #74732
Description
Is your feature request related to a problem? Please describe.
When a gRPC client and gRPC server are local to each other, we avoid going through the network:
cockroach/pkg/rpc/nodedialer/nodedialer.go
Lines 158 to 175 in 775c2b0
| // If we're dialing the local node, don't go through gRPC. | |
| localClient := n.rpcContext.GetLocalInternalClientForAddr(addr.String(), nodeID) | |
| if localClient != nil && !n.testingKnobs.TestingNoLocalClientOptimization { | |
| log.VEvent(ctx, 2, kvbase.RoutingRequestLocallyMsg) | |
| // Create a new context from the existing one with the "local request" field set. | |
| // This tells the handler that this is an in-process request, bypassing ctx.Peer checks. | |
| localCtx := grpcutil.NewLocalRequestContext(ctx) | |
| return localCtx, localClient, nil | |
| } | |
| } | |
| log.VEventf(ctx, 2, "sending request to %s", addr) | |
| conn, err := n.dial(ctx, nodeID, addr, n.getBreaker(nodeID, class), true /* checkBreaker */, class) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| return ctx, TracingInternalClient{InternalClient: roachpb.NewInternalClient(conn)}, err |
As an unfortunate side effect, this bypasses all of our gRPC interceptors:
Lines 169 to 174 in 3f8c928
| // These interceptors will be called in the order in which they appear, i.e. | |
| // The last element will wrap the actual handler. The first interceptor | |
| // guards RPC endpoints for use after Stopper.Drain() by handling the RPC | |
| // inside a stopper task. | |
| var unaryInterceptor []grpc.UnaryServerInterceptor | |
| var streamInterceptor []grpc.StreamServerInterceptor |
and, perhaps less importantly, the client side ones:
Lines 827 to 828 in 3f8c928
| var unaryInterceptors []grpc.UnaryClientInterceptor | |
| var streamInterceptors []grpc.StreamClientInterceptor |
This introduces subtle behavior changes between the two code paths that are
undesirable. The interceptors house tracing and auth logic, for example. In
principle any change to the interceptors requires a second round through the
internal code path, which rarely happens, leading to potential and actual
bugs.
Describe the solution you'd like
It seems desirable for the local vs regular code paths to share as much as
possible while preserving the performance benefit. Since we want to avoid
serialization, we can't use dialer trickery. Instead, the sweet spot might be
linking the respective interceptors into the right places.
Describe alternatives you've considered
Additional context
Jira issue: CRDB-12234