Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imp: refactor the network transport layer #495

Merged

Conversation

cvictory
Copy link
Contributor

现在dubbo和getty耦合度很高,所以先进行第一轮抽象,部分逻辑后续待优化。
做的改动:

  1. 增加exchange层做中转,连接dubbo协议和网络交互。exchange入参出参是invocation和result, 网络层入参出参是request和response
  2. 在dubbo_protocol中增加共用相同连接(客户端访问server,当ip和端口相同,就共用exchange)
  3. 将getty相关的逻辑移到remoting/getty下, 将addr和requestTimeout删除,getty服务管理连接,每个请求的超时,都是请求的入口传入,方便前面提到的连接共享
  4. 抽象codec接口
    5.requestId变成全局,PendingResponse也变成全局

待优化的小的功能点:

  1. codec和remoting.client下后续可以抽象成扩展。现在还是编码形式存在。
  2. codec中的hessian编解码,期望能对hessian进行抽象
  3. getty中的逻辑尽量保持了和原来的逻辑一直,没有做调整,后续看下getty和remoting/getty之间的api层是否可以抽象。

代码主干流程完成,部分异常和分支流程待完善,单元测试待完善。 先提交给大家看下

@flycash
Copy link
Member

flycash commented Apr 26, 2020

@Patrick0308 有时间看看

@Patrick0308
Copy link
Contributor

@flycash OK

Copy link
Member

@pantianying pantianying left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the current program run through the closed-loop state?

return exchangeClient
}

func (client *ExchangeClient) Request(invocation *protocol.Invocation, url common.URL, timeout time.Duration,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

超时时间等需要每次请求再计算下 因为有method维度的配置

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

参考:protocol/dubbo/dubbo_invoker.go:137

client: client,
}
client.SetExchangeClient(exchangeClient)
client.Connect(url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client.Connect需要返回是否成功吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果连接不上,这个ip不能创建invoker。
以增加测试用例。

}

func (server *ExchangeServer) Start() {
(server.Server).Start()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是否必要加(server.Server)中的()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已删除,原来定义成指针,现在不需要了。

Copy link
Contributor

@Patrick0308 Patrick0308 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove useless file and code making easier to review.

@AlexStocks
Copy link
Contributor

@cvictory If u completed ur work and need review, pls give us ur request for review.

@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #495 into feature/dubbo-protocol-restructure will decrease coverage by 0.94%.
The diff coverage is 62.16%.

Impacted file tree graph

@@                          Coverage Diff                           @@
##           feature/dubbo-protocol-restructure     #495      +/-   ##
======================================================================
- Coverage                               67.20%   66.25%   -0.95%     
======================================================================
  Files                                     174      174              
  Lines                                    9261     9330      +69     
======================================================================
- Hits                                     6224     6182      -42     
- Misses                                   2432     2530      +98     
- Partials                                  605      618      +13     
Impacted Files Coverage Δ
config/application_config.go 41.66% <ø> (ø)
remoting/getty/config.go 13.15% <ø> (ø)
remoting/getty/listener.go 40.76% <37.93%> (ø)
remoting/getty/getty_client.go 51.45% <51.45%> (ø)
protocol/dubbo/dubbo_protocol.go 74.72% <60.37%> (-17.12%) ⬇️
protocol/dubbo/dubbo_invoker.go 77.63% <70.00%> (-4.63%) ⬇️
protocol/dubbo/dubbo_codec.go 70.27% <70.27%> (ø)
remoting/getty/readwriter.go 60.00% <85.71%> (ø)
remoting/getty/getty_server.go 63.52% <92.30%> (ø)
protocol/jsonrpc/jsonrpc_invoker.go 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a3c02c...adb55ce. Read the comment docs.

@cvictory cvictory requested a review from pantianying May 9, 2020 06:23
@cvictory
Copy link
Contributor Author

cvictory commented May 9, 2020

@cvictory If u completed ur work and need review, pls give us ur request for review.

please review it again.

@cvictory
Copy link
Contributor Author

@Patrick0308 @pantianying pls review this pr.

remoting.NewCodec("dubbo", codec)
}

// DubboPackage ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add comment for the public structs/funcs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改。

return fmt.Sprintf("DubboPackage: Header-%v, Path-%v, Body-%v", p.Header, p.Service, p.Body)
}

// Marshal ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改。

return bytes.NewBuffer(pkg), nil
}

// Unmarshal ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改。

protocol/dubbo/dubbo_codec.go Show resolved Hide resolved
protocol/dubbo/dubbo_codec.go Show resolved Hide resolved
"github.com/apache/dubbo-go/common"
)

type Server interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comment for the public structs/funcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改。

"strconv"
"time"

hessian "github.com/apache/dubbo-go-hessian2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls using anther import block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改。

return bytes.NewBuffer(pkg), nil
}

// Unmarshal ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comment for public strucs/funcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is unit test.

"math/rand"
"time"

"github.com/apache/dubbo-go/remoting"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls using three import import blocks as other dubbo-go files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改。

rand.Seed(time.Now().UnixNano())
}

// SetClientConf ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add comment for public funcs/structs in this file.

protocol/dubbo/dubbo_codec.go Show resolved Hide resolved
protocol/dubbo/dubbo_codec.go Show resolved Hide resolved
remoting/getty/dubbo_codec_for_test.go Outdated Show resolved Hide resolved
@hxmhlt
Copy link
Contributor

hxmhlt commented May 12, 2020

pls add some ut. The ut coverage is lower.

@cvictory cvictory changed the base branch from master to feature/dubbo-protocol-restructure May 13, 2020 13:21
codec = make(map[string]Codec, 2)
}

func NewCodec(protocol string, codecTmp Codec) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe It would be better to replace new with register?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改了。

codec map[string]Codec
)

func init() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to initialize in init
example:
var codec=make( map[string]Codec)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改了

client.SetExchangeClient(exchangeClient)
if client.Connect(url) != nil {
//retry for a while
time.Sleep(1 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's necessary to try again, there's no need to wait for a while

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

连接有可能瞬时的网络问题,所以sleep了100ms


if pendingResponse.Callback == nil {
pendingResponse.Err = pendingResponse.response.Error
pendingResponse.Done <- struct{}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use close(pendingResponse.Done)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个和原来是一致的。为什么要直接关闭?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一般chan 作为一次性关闭使用都是close,可以让全部协程<-Done 都触发。只发送一个信号,如果该Done在多个协程使用了,只能关闭一个协程。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个只有一个地方使用,这个地方就是接受相应请求。后续可以考虑。

if session == nil {
return errSessionNotExist
}
defer func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这样的写法有点奇怪,这里没有必要用defer吧。。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

删除了。

@AlexStocks AlexStocks changed the title 重构网络模块 Imp: refactor the network transport layer May 15, 2020
@@ -141,6 +141,29 @@ func (r *RPCInvocation) SetCallBack(c interface{}) {
r.callBack = c
}

func (r *RPCInvocation) ServiceKey() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add ServiceKey here? is url.servicekey wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

因为回来的时候,所有参数放到了rpcInvocation中。按照dubbo java做法,其实invocation下会有invoker,再通过invoker获取url。这里暂时不去完全适配。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个invocation里面不是有invoker吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invoker需要去获取,暂时还没有。 原来逻辑里是通过构建一个url,通过url.ServiceKey来获取服务标识。

protocol/dubbo/dubbo_protocol_test.go Show resolved Hide resolved
requestTimeout = t
exchangeClient := getExchangeClient(url)
if exchangeClient == nil {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when get nil exchangeClient, if we should throw a error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type Protocol interface {
	Export(invoker Invoker) Exporter
	Refer(url common.URL) Invoker
	Destroy()
}

这个接口的Refer接口,没有error返回值,所以要么只能panic,要么只能返回nil.

因为连接不上认为是比较常见的问题,所以返回nil来支持。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java中是往上抛异常。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so add warning log?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

加一句日志会更好一点。不过我们后面应该考虑一下要不要修改这个返回值,支持返回error。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我增加一个warning的log吧。

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #495 into feature/dubbo-protocol-restructure will decrease coverage by 1.11%.
The diff coverage is 60.08%.

Impacted file tree graph

@@                          Coverage Diff                           @@
##           feature/dubbo-protocol-restructure     #495      +/-   ##
======================================================================
- Coverage                               67.20%   66.09%   -1.12%     
======================================================================
  Files                                     174      174              
  Lines                                    9261     9343      +82     
======================================================================
- Hits                                     6224     6175      -49     
- Misses                                   2432     2544     +112     
- Partials                                  605      624      +19     
Impacted Files Coverage Δ
common/url.go 71.15% <0.00%> (ø)
config/application_config.go 41.66% <ø> (ø)
protocol/jsonrpc/jsonrpc_invoker.go 100.00% <ø> (ø)
...rotocol/protocolwrapper/protocol_filter_wrapper.go 48.64% <0.00%> (-2.78%) ⬇️
remoting/getty/config.go 13.15% <ø> (ø)
remoting/getty/listener.go 39.49% <37.93%> (ø)
remoting/getty/getty_client.go 44.68% <44.68%> (ø)
protocol/dubbo/dubbo_protocol.go 71.69% <58.82%> (-20.14%) ⬇️
protocol/dubbo/dubbo_invoker.go 77.63% <70.00%> (-4.63%) ⬇️
protocol/dubbo/dubbo_codec.go 70.27% <70.27%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a3c02c...5ba2307. Read the comment docs.

@@ -40,7 +37,7 @@ func (*ApplicationConfig) Prefix() string {
return constant.DUBBO + ".application."
}

// Id ...
// ID ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if u do not wanna add comment here, pls add // nolint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. It is keep the same as before.

import (
"github.com/apache/dubbo-go/common/constant"
"github.com/creasty/defaults"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -19,7 +19,10 @@ package dubbo

import (
"context"
"github.com/apache/dubbo-go/config"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -48,21 +51,31 @@ var (
attachmentKey = []string{constant.INTERFACE_KEY, constant.GROUP_KEY, constant.TOKEN_KEY, constant.TIMEOUT_KEY}
)

// DubboInvoker ...
// DubboInvoker. It is implement of protocol.Invoker. One dubboInvoker refer to one service and ip.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DubboInvoker is implement of protocol.Invoker, a DubboInvoker refer to one service and ip.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -83,6 +96,9 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
defer atomic.AddInt64(&(di.reqNum), -1)

inv := invocation.(*invocation_impl.RPCInvocation)
// init param
inv.SetAttachments(constant.PATH_KEY, di.GetUrl().GetParam(constant.INTERFACE_KEY, ""))
inv.SetAttachments(constant.VERSION_KEY, di.GetUrl().GetParam(constant.VERSION_KEY, ""))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add to attachmentKey?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because there params are removed in previous process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好主意。

}
}
if result.Err == nil {
result.Rest = inv.Reply()
result.Attrs = response.atta
result.Attrs = result.Attachments()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you set attachments to result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我的意思是,result.Attachments() 取出来的就是result.Attrs,你这里取自己给自己赋值?而且没看到result的attrs有被赋值,response.atta 原来的这个内容在哪里呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

en, 取错对象了,应用用rest.

@@ -58,7 +57,9 @@ func TestDubboInvoker_Invoke(t *testing.T) {
res := invoker.Invoke(context.Background(), inv)
assert.NoError(t, res.Error())
assert.Equal(t, User{Id: "1", Name: "username"}, *res.Result().(*User))
assert.Equal(t, "test_value", res.Attachments()["test_key"]) // test attachments for request/response
//result will not contain attachment
// attachment in result it is useless.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have removed.

Copy link
Member

@pantianying pantianying left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

requestTimeout = t
exchangeClient := getExchangeClient(url)
if exchangeClient == nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

加一句日志会更好一点。不过我们后面应该考虑一下要不要修改这个返回值,支持返回error。


func getExchangeClient(url common.URL) *remoting.ExchangeClient {
clientTmp, ok := exchangeClientMap.Load(url.Location)
if !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这一步实际上存在并发隐患。如果多线程进来,同时发现没有,就会同时进来这里,于是创建多个,而后者覆盖前者Store调用的结果。修改的建议是使用double-check在发现!ok之后加锁再尝试取一次,取不到则创建。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增加了一层锁,可以看下:用了syncMap

return exchangeClientTmp
}
exchangeClient, ok := clientTmp.(*remoting.ExchangeClient)
if !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

什么情况下会执行到!ok里面?我有点没看懂……

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没有情形,会走到这个里面。 默认放进去的都是这个类型。

@@ -54,7 +53,7 @@ func (ji *JsonrpcInvoker) Invoke(ctx context.Context, invocation protocol.Invoca
url := ji.GetUrl()
req := ji.client.NewRequest(url, inv.MethodName(), inv.Arguments())
ctxNew := context.WithValue(ctx, constant.DUBBOGO_CTX_KEY, map[string]string{
"X-Proxy-Id": "dubbogo",
"X-Proxy-ID": "dubbogo",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里修改之后,是否会出现新版本的jsonrpc客户端调过去老版本的jsonrpc服务端失败的问题?因为老版本的可能只能识别"X-Proxy-Id"。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

恢复。

}

func SequenceId() uint64 {
// increse 2 for every request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最好把为什么+2而不是+1这个讲清楚。这是一个很tricky的事情=。=

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

加了注释,client-》 server期望用偶数,server-》client用奇数。 这个没有强制,只是为了更加方便区分。

// NewRequest
func NewRequest(version string) *Request {
return &Request{
ID: int64(SequenceId()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里看来,为什么sequenceId不直接生成64位?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

之前想用uint表示,后面想了下,还是用int64吧。 和java之类的通信,会溢出。

p.Service.Method = invocation.MethodName()

timeout, err := strconv.Atoi(invocation.AttachmentsByKey(constant.TIMEOUT_KEY, "3000"))
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would better define a constant like default timeout for 3000.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改

if err != nil {
// it will be wrapped in readwrite.Write .
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perrors.WithStack(err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改

Event: pkg.Header.Type&hessian.PackageHeartbeat != 0x00,
}
if pkg.Header.Type&hessian.PackageHeartbeat == 0x00 {
// convert params of request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No go fmt? '&' 前后应该有空格才对。建议ide调成自动format

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有的,主要是有优先级比较的缘故(& 和 ==)吧。加了个括号就好了。。。

argBuf := new(bytes.Buffer)
for i := 0; i < 400; i++ {
argBuf.WriteString("击鼓其镗,踊跃用兵。土国城漕,我独南行。从孙子仲,平陈与宋。不我以归,忧心有忡。爰居爰处?爰丧其马?于以求之?于林之下。死生契阔,与子成说。执子之手,与子偕老。于嗟阔兮,不我活兮。于嗟洵兮,不我信兮。")
argBuf.WriteString("击鼓其镗,踊跃用兵。土国城漕,我独南行。从孙子仲,平陈与宋。不我以归,忧心有忡。爰居爰处?爰丧其马?于以求之?于林之下。死生契阔,与子成说。执子之手,与子偕老。于嗟阔兮,不我活兮。于嗟洵兮,不我信兮。")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think Chinese is ok. Pls mod to English...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是之前的用例,我移动了个地方。 中文也测试下,可以吧。 我加个注释?


func doHandleRequest(rpcInvocation *invocation.RPCInvocation) protocol.RPCResult {
exporter, _ := dubboProtocol.ExporterMap().Load(rpcInvocation.ServiceKey())
result := protocol.RPCResult{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is dubboProtocol.ExporterMap() may be nil?

Copy link
Contributor Author

@cvictory cvictory Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

除非是非法流量。
处理的逻辑统一在export == nil 里进行处理。

}
}
//FIXME atomic operation
cl.init = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need atomic.Bool

return er
}
request := NewRequest("2.0.2")
request.Data = invocation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.0.2 means what?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.0.2 means what?

Pls use constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.0.2是协议版本

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个pr太大了,不做调整了。 我去社区提个issue吧,很多地方都有。

Copy link
Contributor

@fangyincheng fangyincheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@hxmhlt hxmhlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hxmhlt hxmhlt merged commit a10ff4d into apache:feature/dubbo-protocol-restructure Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants