No-SilverBullet commented on code in PR #2900:
URL: https://github.com/apache/dubbo-go/pull/2900#discussion_r2113871374
##########
protocol/triple/client.go:
##########
@@ -206,6 +197,13 @@ func newClientManager(url *common.URL) (*clientManager,
error) {
tlsFlag = true
}
+ cliKeepAliveOpts, keepAliveInterval, keepAliveTimeout,
genKeepAliveOptsErr := genKeepAliveOpts(url)
+ if genKeepAliveOptsErr != nil {
+ return nil, genKeepAliveOptsErr
Review Comment:
can we add a error log in here
##########
options.go:
##########
@@ -378,8 +378,27 @@ func WithMetadataServiceProtocol(protocol string)
InstanceOption {
}
}
-func WithProtocol(opts ...protocol.Option) InstanceOption {
- proOpts := protocol.NewOptions(opts...)
+// TODO: deal this fuction
+// this function I want handle the protocol.Option which
+// both server and client can use together.
+// like:
+//
+// func WithProtocol(opts ...protocol.Option) InstanceOption {
+// proOpts := protocol.NewOptions(opts...)
+//
+// log.Warnf("proOpts: %+v", proOpts)
+//
+// return func(insOpts *InstanceOptions) {
+// if insOpts.Protocols == nil {
+// insOpts.Protocols =
make(map[string]*global.ProtocolConfig)
+// }
+// insOpts.Protocols[proOpts.ID] = proOpts.Protocol
+// }
+// }
+//
+// but now only work in server side fot compat old API.
Review Comment:
typo fot
##########
global/triple_config.go:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package global
+
+// TODO: Should the server and client configurations be separated?
Review Comment:
i think sepatate the client and server may be more clear
##########
protocol/triple/server.go:
##########
@@ -54,18 +55,18 @@ import (
// provide functionality.
type Server struct {
triServer *tri.Server
- cfg *ServerOptions
+ cfg *global.TripleConfig
mu sync.RWMutex
services map[string]grpc.ServiceInfo
}
// NewServer creates a new TRIPLE server.
// triServer would not be initialized since we could not get configurations
here.
-func NewServer(opts ...ServerOption) *Server {
- newSrvOpts := defaultServerOptions()
- newSrvOpts.init(opts...)
+// NOTE: Now everything changed, we can initialized triServer now.
Review Comment:
remove the old comment
##########
protocol/triple/client.go:
##########
@@ -282,6 +280,49 @@ func newClientManager(url *common.URL) (*clientManager,
error) {
}, nil
}
+func genKeepAliveOpts(url *common.URL) ([]tri.ClientOption, time.Duration,
time.Duration, error) {
+ var cliKeepAliveOpts []tri.ClientOption
+
+ // set max send and recv msg size
+ maxCallRecvMsgSize := constant.DefaultMaxCallRecvMsgSize
+ if recvMsgSize, err :=
humanize.ParseBytes(url.GetParam(constant.MaxCallRecvMsgSize, "")); err == nil
&& recvMsgSize > 0 {
+ maxCallRecvMsgSize = int(recvMsgSize)
+ }
+ cliKeepAliveOpts = append(cliKeepAliveOpts,
tri.WithReadMaxBytes(maxCallRecvMsgSize))
+ maxCallSendMsgSize := constant.DefaultMaxCallSendMsgSize
+ if sendMsgSize, err :=
humanize.ParseBytes(url.GetParam(constant.MaxCallSendMsgSize, "")); err == nil
&& sendMsgSize > 0 {
+ maxCallSendMsgSize = int(sendMsgSize)
+ }
+ cliKeepAliveOpts = append(cliKeepAliveOpts,
tri.WithSendMaxBytes(maxCallSendMsgSize))
+
+ // set keepalive interval and keepalive timeout
+ // Deprecated：use tripleconfig
+ // TODO: remove KeepAliveInterval and KeepAliveInterval in version 4.0.0
+ keepAliveInterval := url.GetParamDuration(constant.KeepAliveInterval,
constant.DefaultKeepAliveInterval)
+ keepAliveTimeout := url.GetParamDuration(constant.KeepAliveTimeout,
constant.DefaultKeepAliveTimeout)
+
+ tripleConfRaw, ok := url.GetAttribute(constant.TripleConfigKey)
+ if ok {
+ var parseErr error
+ tripleConf := tripleConfRaw.(*global.TripleConfig)
+ // TODO: handle ParseDuration error
+ if tripleConf != nil && tripleConf.KeepAliveInterval != "" {
+ keepAliveInterval, parseErr =
time.ParseDuration(tripleConf.KeepAliveInterval)
+ if parseErr != nil {
+ return nil, 0, 0, parseErr
+ }
+ }
+ if tripleConf != nil && tripleConf.KeepAliveTimeout != "" {
+ keepAliveTimeout, parseErr =
time.ParseDuration(tripleConf.KeepAliveTimeout)
+ if parseErr != nil {
+ return nil, 0, 0, parseErr
+ }
Review Comment:
judge the if tripleConf != nil in the begining
if tripleConf == nil{
return XXX
}
if tripleConf.KeepAliveInterval != ""{
xxx
}
##########
protocol/triple/server.go:
##########
@@ -174,19 +175,46 @@ func (s *Server) RefreshService(invoker base.Invoker,
info *common.ServiceInfo)
}
func getHanOpts(url *common.URL) (hanOpts []tri.HandlerOption) {
+ var tripleConf *global.TripleConfig
+
+ tripleConfRaw, ok := url.GetAttribute(constant.TripleConfigKey)
+ if ok {
+ tripleConf = tripleConfRaw.(*global.TripleConfig)
+ }
+
+ // Deprecated：use TripleConfig
+ // TODO: remove MaxServerSendMsgSize and MaxServerRecvMsgSize when
version 4.0.0
var err error
maxServerRecvMsgSize := constant.DefaultMaxServerRecvMsgSize
if recvMsgSize, convertErr :=
humanize.ParseBytes(url.GetParam(constant.MaxServerRecvMsgSize, ""));
convertErr == nil && recvMsgSize != 0 {
maxServerRecvMsgSize = int(recvMsgSize)
}
hanOpts = append(hanOpts, tri.WithReadMaxBytes(maxServerRecvMsgSize))
+ if tripleConf != nil && tripleConf.MaxServerRecvMsgSize != "" {
+ logger.Warnf("MaxServerRecvMsgSize: %v",
tripleConf.MaxServerRecvMsgSize)
+ if recvMsgSize, convertErr :=
humanize.ParseBytes(tripleConf.MaxServerRecvMsgSize); convertErr == nil &&
recvMsgSize != 0 {
+ maxServerRecvMsgSize = int(recvMsgSize)
+ }
+ hanOpts = append(hanOpts,
tri.WithReadMaxBytes(maxServerRecvMsgSize))
+ }
+
+ // Deprecated：use TripleConfig
Review Comment:
same here, judge the if tripleConf != nil in the begining
##########
protocol/triple/options.go:
##########
@@ -17,30 +17,78 @@
package triple
-type ServerOptions struct {
+import (
+ "time"
+)
+
+import (
+ "dubbo.apache.org/dubbo-go/v3/global"
+)
+
+// TODO: The triple options for the server and client are mixed together now.
+// We need to find a way to separate them later.
+
+type Options struct {
+ Triple *global.TripleConfig
}
-func defaultServerOptions() *ServerOptions {
- return &ServerOptions{}
+func defaultOptions() *Options {
+ return &Options{Triple: global.DefaultTripleConfig()}
}
-func NewServerOptions(opts ...ServerOption) *ServerOptions {
- defSrvOpts := defaultServerOptions()
+func NewOptions(opts ...Option) *Options {
+ defSrvOpts := defaultOptions()
for _, opt := range opts {
opt(defSrvOpts)
}
return defSrvOpts
}
-func (srvOpts *ServerOptions) init(opts ...ServerOption) {
- for _, opt := range opts {
- opt(srvOpts)
+type Option func(*Options)
+
+// WithKeepAlive sets the keep-alive interval and timeout for the Triple
protocol.
+// interval: The duration between keep-alive pings.
+// timeout: The duration to wait for a keep-alive response before considering
the connection dead.
+// If not set, default interval is 10s, default timeout is 20s.
Review Comment:
Gooooooooood comment!
:)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
