feat: sort out triple logic and fix comments #2454
Conversation
|
"XXX_TRIPLE_GO_INTERFACE_NAME" lint problem solved in #2456 should be tagged to be ignored since old triple generated code could not be changed. |
Could you please add an ignore annotation for it? |
|
Or we need protoc-gen-triple plugin to suport it? |
It is only a golangci-lint problem and we can not fix it referring to #2456 (define a custom type) since stub code generated by protoc-gen-go-triple makes use of "XXX_TRIPLE_GO_INTERFACE_NAME" directly. I would add an ignoring annotation. |
DMwangnima
changed the title
|
Kudos, SonarCloud Quality Gate passed!
|
| // consumer config client connectTimeout | ||
| //connectTimeout := config.GetConsumerConfig().ConnectTimeout | ||
| // set timeout | ||
| timeout := url.GetParamDuration(constant.TimeoutKey, "") |
There was a problem hiding this comment.
If I understand it right, this url contains the complete configuration of a specific service? I think we need to call something like below to get the method level timeout configuration:
timeout := url.GetMethodParamDuration(constant.TimeoutKey, "")
There was a problem hiding this comment.
What's the default timeout value for (constant.TimeoutKey, "")?
There was a problem hiding this comment.
I just realized something. Are we in the middle of an RPC invoke here? If not, we might not be able to get the method name needed.
There was a problem hiding this comment.
The specific method level timeout (optional) passed in with ctx in TripleInvoker.invoke(ctx), then to clientManager.callUnary/callClientStream(). If timeout presents in ctx, then it will be used as the timeout value; If not, the default value calculated here by timeout := url.GetParamDuration(constant.TimeoutKey, "") will be used.
Is the above process right?
There was a problem hiding this comment.
The specific method level timeout (optional) passed in with
ctxinTripleInvoker.invoke(ctx), then toclientManager.callUnary/callClientStream(). If timeout presents in ctx, then it will be used as the timeout value; If not, the default value calculated here bytimeout := url.GetParamDuration(constant.TimeoutKey, "")will be used.Is the above process right?
Yes, that is. The ctx used by users would have highest priority. It is more ideal for uses to use ctx to specify timeout for method level since they can use cancel function directly.
There was a problem hiding this comment.
I just realized something. Are we in the middle of an
RPC invokehere? If not, we might not be able to get themethod nameneeded.
We are in the process of Refer, i.e. initialize client and some general configurations.
There was a problem hiding this comment.
The specific method level timeout (optional) passed in with
ctxinTripleInvoker.invoke(ctx), then toclientManager.callUnary/callClientStream(). If timeout presents in ctx, then it will be used as the timeout value; If not, the default value calculated here bytimeout := url.GetParamDuration(constant.TimeoutKey, "")will be used.
Is the above process right?Yes, that is. The
ctxused by users would have highest priority. It is more ideal for uses to usectxto specify timeout for method level since they can usecancelfunction directly.
I see, this sounds reasonable to me.
One more thing, we need to check the method level timeout if ctx is not set, then put method timeout into ctx.
| func NewDubbo3Invoker(url *common.URL) (*DubboInvoker, error) { | ||
| rt := config.GetConsumerConfig().RequestTimeout | ||
|
|
||
| timeout := url.GetParamDuration(constant.TimeoutKey, rt) |
There was a problem hiding this comment.
Should also consider method timeout.
This PR aims to sort out Triple logic and support compatibility between Triple adaptation code and Dubbo3 generated client stub code