WIP: feat: add binary-grpc-generic-call #1242
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1242 +/- ##
===========================================
- Coverage 68.46% 68.35% -0.11%
===========================================
Files 284 285 +1
Lines 20618 20654 +36
===========================================
+ Hits 14117 14119 +2
- Misses 5319 5351 +32
- Partials 1182 1184 +2 ☔ View full report in Codecov by Sentry. |
| @@ -42,6 +42,11 @@ func NewClientWithServiceInfo(destService string, g generic.Generic, svcInfo *se | |||
| options = append(options, client.WithDestService(destService)) | |||
| options = append(options, opts...) | |||
|
|
|||
| if g.PayloadCodecType() == serviceinfo.Protobuf { | |||
There was a problem hiding this comment.
这里别通过service info来判断grpc，因为thrift协议场景也可以走pb序列化
建议直接判断是不是grpc generic那个结构体?
| return binaryG.grpcSvcName | ||
| } | ||
|
|
||
| func (g *binaryGrpcGeneric) Framed() bool { |
There was a problem hiding this comment.
framed为啥是true?可以看看这个调用是哪里用到的。framed一般是指thrift协议的特性
| } | ||
|
|
||
| func (g *binaryGrpcGeneric) PayloadCodec() remote.PayloadCodec { | ||
| return nil |
There was a problem hiding this comment.
现在返回nil是因为你复用了grpcCodec没单独搞个payload吧？后面觉得可能要把grpcCodec的代码适当拆分下?
There was a problem hiding this comment.
如果要搞成和thrift那边一样加payloadCodec，可能你的grpcCodec那里原来那个switch的分支都要改改
| @@ -42,6 +42,11 @@ func NewClientWithServiceInfo(destService string, g generic.Generic, svcInfo *se | |||
| options = append(options, client.WithDestService(destService)) | |||
| options = append(options, opts...) | |||
|
|
|||
| if g.PayloadCodecType() == serviceinfo.Protobuf { | |||
| svcInfo.Extra["grpcGeneric"] = true | |||
| svcInfo.ServiceName = generic.GetGrpcSvcName(g) | |||
There was a problem hiding this comment.
我觉得可以把extra去掉?泛化场景都默认尝试填充service name
thrift场景之前没有填也没有传递，所以就算是空的也不影响?
There was a problem hiding this comment.
这个grpcGeneric的字段是我新加的，主要是原来的逻辑直接通过设service name为“$genericCall”来判断是泛化场景的，grpc要求加了service name就只能加一个新的字段了
There was a problem hiding this comment.
嗯。以后可能 thrift 场景也需要。demo 就先这样吧
| ) | ||
|
|
||
| // BinaryGrpcGeneric raw grpc binary Generic | ||
| func BinaryGrpcGeneric(grpcSvcName string) Generic { |
There was a problem hiding this comment.
不对，我又想了下，service name还是需要每次随着call的时候传入，泛化grpc client需要有每次调用不同service.mehtod的能力
There was a problem hiding this comment.
那可能需要改造一下GenericCall()，或者加个callOption？看看怎么搞和thrift比较兼容
| @@ -132,6 +133,15 @@ func (c *grpcCodec) Encode(ctx context.Context, message remote.Message, out remo | |||
| payload, err = proto.Marshal(t) | |||
| case protobuf.ProtobufMsgCodec: | |||
| payload, err = t.Marshal(nil) | |||
| case *generic.Args: | |||
There was a problem hiding this comment.
编码不一定是 Protobuf，也可能是 thrift 或者任意数据
There was a problem hiding this comment.
更通用的方式是，generic.Args 定义为接口，只要实现了这个接口的类型都可以在这里用
There was a problem hiding this comment.
例如
type Args interface {
Marshal(ctx context.Context) ([]byte, error)
}
There was a problem hiding this comment.
还有应该要提供一种方式，可以让client修改 content-type，目前默认是
application/grpc
对于 thrift，则是按照 grpc 的规范增加了个 contentSubtype（可以在代码中搜这个变量名）
application/grpc+thrift
| @@ -194,6 +204,9 @@ func (c *grpcCodec) Decode(ctx context.Context, message remote.Message, in remot | |||
| return proto.Unmarshal(d, t) | |||
| case protobuf.ProtobufMsgCodec: | |||
| return t.Unmarshal(d) | |||
| case *generic.Result: | |||
There was a problem hiding this comment.
同上，编码不一定是 Protobuf，也可能是 thrift 或者任意数据
What type of PR is this?
feat
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en: add BinaryGrpcGeneric type for generic client to support grpc in unary generic call.
users can now use something like
genericclient.NewClient("a.b.c", generic.BinaryGrpcGeneric("package.service"), ...)to create generic client that supports grpc.
zh(optional):
(Optional) Which issue(s) this PR fixes:
(optional) The PR that updates user documentation: