grpc/service: Refactor middleware #8849
Conversation
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
size/XXL
ti-chi-bot
bot
added
the
do-not-merge/work-in-progress
|
|
||
| type forwardFn func(ctx context.Context, client *grpc.ClientConn, request any) (any, error) | ||
|
|
||
| var forwardFns = map[string]forwardFn{ |
There was a problem hiding this comment.
Isn't it easier to manage after the modifications compared to the previous definition scattered across each function? Could you elaborate on your viewpoint?
There was a problem hiding this comment.
There are too many hardcode things and you still need to add them one by one.
There was a problem hiding this comment.
From the current implementation, it seems that hardcoding cannot be avoided. But this has nothing to do with this PR (it was hardcoded before as well). Based on the existing changes, where do you think it has become more complex?
There was a problem hiding this comment.
We don't need to care about the function name for now.
There was a problem hiding this comment.
If we need to maintain it, we have to pay attention to the function names, right? I think we should know which functions support a certain middleware and which do not.
don't need to care about the function name
Or do you mean that all functions should support the forward middleware, so we don't need to worry about the function names? If that's the case, I don't think the impact is significant; placing a map here doesn't imply that we need to be overly concerned about function names, and it doesn't add much burden. If we didn't have this map, we would still need to define a forwardFn inside the function when adding new functions, which makes no difference.
If we still hold different views, perhaps we should listen to the opinions of other members. cc @JmPotato @nolouch @lhy1024
|
I will fix the problem from ci after discussion |
What problem does this PR solve?
Issue Number: Close #8850
What is changed and how does it work?
Check List
Tests
Release note