introduce RPC interceptor mechanism #389
Conversation
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
|
@zhongzc PTAL |
| @@ -203,6 +205,13 @@ func (s *KVSnapshot) BatchGet(ctx context.Context, keys [][]byte) (map[string][] | |||
| ctx = context.WithValue(ctx, retry.TxnStartKey, s.version) | |||
| bo := retry.NewBackofferWithVars(ctx, batchGetMaxBackoff, s.vars) | |||
|
|
|||
| if s.interceptor != nil { | |||
There was a problem hiding this comment.
Seems that there are some places missed calling interceptor, for example, batch coprocessor. Although currently we don't need it, it would be great to have that from the perspective of client-go. If don't know how to deal with it correctly, then at least add a TODO.
There was a problem hiding this comment.
I think users need to know that they are using KVTxn or KVSnapshot, and the effective area of Interceptor is limited to a specific KVTxn or KVSnapshot.
So when users don't need to use abstract transactions or snapshots, but need to send requests directly (e.g. copr), they need to explicitly add the Interceptor to the ctx associated with the request. That's why we need to provide CtxWithInterceptor API to users.
There was a problem hiding this comment.
In the implementation of SQL Exec Count, we have done this to count the distribution of copr requests.
There was a problem hiding this comment.
Do you mean that actually batch coprocessor is not handled by client-go? Then I think it would be fine.
There was a problem hiding this comment.
All coprocessor requests will eventually be processed by ClientHelper in client-go, but ClientHelper is just a simple wrap of inner client, and only provide a SendReq method. In this pattern, I think it is reasonable to let user explicitly call WithCtxInterceptor on ctx.
The ctx of transaction and snapshot is constructed internally, so we provide high-level AddInterceptor() for transaction and snapshot; The ctx requested by the coprocessor is constructed by the user and passed in through ClientHelper.SendReq(ctx), so we let user to call WithCtxInterceptor explicitly.
There is a unified principle here: Where the context.Context constructed, where the Interceptor bound.
| @@ -118,6 +118,12 @@ func (s *Scanner) Next() error { | |||
| if !s.valid { | |||
| return errors.New("scanner iterator is invalid") | |||
| } | |||
| if s.snapshot.interceptor != nil { | |||
There was a problem hiding this comment.
I wonder what's the principle to place these interceptor settings.
There was a problem hiding this comment.
The principle is: trace the origin of the ctx associated with the inner gRPC request.
In this example, ctx is created on L117:
bo := retry.NewBackofferWithVars(context.WithValue(context.Background(), retry.TxnStartKey, s.snapshot.version), scannerNextMaxBackoff, s.snapshot.vars)So we immediately bind the Interceptor on L121:
if s.snapshot.interceptor != nil {
// User has called snapshot.SetInterceptor() to explicitly set an interceptor, we
// need to bind it to ctx so that the internal client can perceive and execute
// it before initiating an RPC request.
bo.SetCtx(tikvrpc.SetInterceptorIntoCtx(bo.GetCtx(), s.snapshot.interceptor))
}Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
There was a problem hiding this comment.
The rest LGTM. @disksing @sticnarf @andylokandy PTAL, thanks!
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
|
The integration test of this PR always fails. Can someone help me confirm if it is caused by my integration test case ( |
Yeah, or course. The failure output indicate that it's the interceptor test that failed: There are many "[ERROR]" logs in the wild, but they are unrelated and are produced in normal test process. You need to expand the full output log to see the actual failure cases. |
From the stack, we can see the interceptor is called twice in the test. One is in the |
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
|
Emm, I don't think it a good idea that this interceptor integration test is only effective with TiKV. People usually don't start a TiKV to run these integration tests during development. What do you think about moving the interceptor for |
Signed-off-by: mornyx <mornyx.z@gmail.com>
Looks good, I have updated~ |
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
|
/run-all-tests |
|
Can you reproduce the following error locally? |
Yes, I reproduced it. It seems that this problem has existed before, all open PRs have this error. |
Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
|
Please have a test to cover the change from 0011200 |
done for this |
|
Hi @sticnarf , could you please help to merge this PR? Thank you |
What's changed
Introduce RPC Interceptor mechanism. This is originally designed for TopSQL functionality, but its design is general purpose.
Original PR which introduced this feature:
What is
RPCInterceptorRPCInterceptoris used to decorate the RPC requests to TiKV.The definition of an interceptor is: Given an
RPCInterceptorFunc, we will get the decoratedRPCInterceptorFuncwith additional logic before and after the execution of the givenRPCInterceptorFunc.How to implement an
RPCInterceptorWe can implement an
RPCInterceptorlike this:Or you want to inject some dependent modules:
Signed-off-by: mornyx mornyx.z@gmail.com