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

Ftr: [refer dubbo 2.7.6] attachment type from map[string]stiring to map[string]interface{} #713

Merged
merged 18 commits into from
Sep 7, 2020

Conversation

cvictory
Copy link
Contributor

1.attachment类型从attachment type from map[string]stiring 到 map[string]interface{} ,和dubbo java对齐
2.功能分成两部分,一部分会在dubbo-go里提交,一部分需要修改hessian.

dubbo-go-hessian :apache/dubbo-go-hessian2#218

这个pr依赖dubbo-go-hessian merge完成之后,修改依赖的版本之后,才能通过CI。提交上来是为了方便review的时候一起看。
先忽略CI

@cvictory cvictory changed the base branch from master to develop August 15, 2020 04:10
Copy link
Member

@zouyx zouyx left a comment

Choose a reason for hiding this comment

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

Travis still failing, check it again pls.
Thanks.

@zouyx zouyx added this to the 1.5.2 milestone Aug 17, 2020
@zouyx zouyx added the enhancement New feature or request label Aug 17, 2020
@cvictory
Copy link
Contributor Author

Travis still failing, check it again pls.
Thanks.

先等另外一个pr review通过之后,我修改下依赖,来跑CI。 因为两个pr有依赖。

@zouyx zouyx changed the base branch from develop to 1.5.1 August 18, 2020 02:13
@zouyx zouyx changed the base branch from 1.5.1 to develop August 18, 2020 02:14
@zouyx
Copy link
Member

zouyx commented Aug 18, 2020

Travis still failing, check it again pls.
Thanks.

先等另外一个pr review通过之后,我修改下依赖,来跑CI。 因为两个pr有依赖。

ok,thanks a lot

require (
github.com/apache/dubbo-go-hessian2 v1.6.0-rc1.0.20200819060303-9b32032784f6
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是为了跑通集成测试,等hessian发布最新版本之后,可以删除。

@zouyx zouyx changed the title [refer dubbo 2.7.6] attachment type from map[string]stiring to map[string]interface{} Ftr: [refer dubbo 2.7.6] attachment type from map[string]stiring to map[string]interface{} Aug 20, 2020
@cvictory
Copy link
Contributor Author

@joeyzhouy zhou 帮忙协调下review。

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.

https://github.com/cvictory/dubbo-go/blob/93bfb98f12028617c62509505a60bfd5ab3c4d08/common/proxy/proxy.go#L149

这里还没有调整,这里要评估下之前用户通过ctx传进的map[string]string的attachment

@cvictory
Copy link
Contributor Author

https://github.com/cvictory/dubbo-go/blob/93bfb98f12028617c62509505a60bfd5ab3c4d08/common/proxy/proxy.go#L149

这里还没有调整,这里要评估下之前用户通过ctx传进的map[string]string的attachment

这个暂时不要动吧。 面向应用开发者还是不动先,面向框架扩展的得升级。

@@ -19,6 +19,8 @@ package proxy

import (
"context"
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.

OMG. move it to the second import block, pls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -19,14 +19,14 @@ package dubbo

import (
"context"
"github.com/opentracing/opentracing-go"
Copy link
Member

Choose a reason for hiding this comment

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

move this into github.com imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Attachments() map[string]string
// AttachmentsByKey gets attachment by key , if nil then return default value
Attachments() map[string]interface{}
// AttachmentsByKey gets attachment by key , if nil then return default value. (It will be discarded)
Copy link
Member

Choose a reason for hiding this comment

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

change it to 'deprecated in the future'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -151,6 +151,10 @@ func (p *Proxy) Implement(v common.RPCService) {
for k, value := range m {
inv.SetAttachments(k, value)
}
} else if m2, ok2 := atm.(map[string]interface{}); ok2 {
Copy link
Member

Choose a reason for hiding this comment

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

we'd better leave some comments here so that others can understand the context better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

done

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the comment should be aligned with the else branch in the same line or under the else branch if it is too long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -151,6 +151,10 @@ func (p *Proxy) Implement(v common.RPCService) {
for k, value := range m {
inv.SetAttachments(k, value)
}
} else if m2, ok2 := atm.(map[string]interface{}); ok2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the comment should be aligned with the else branch in the same line or under the else branch if it is too long.


package dubbo

import (
Copy link
Contributor

Choose a reason for hiding this comment

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

pls do not place dubbo-go package and other packages in the same 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.

done

ErrIllegalPackage = perrors.New("illegal package!")
)

// DescRegex ...
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. If u do not wanna add comment, use "// nolint" instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

PackageType_BitSize = 0x2f
)

// PackageType ...
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. If u do not wanna add comment, use "// nolint" instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cvictory cvictory merged commit 49c005a into apache:develop Sep 7, 2020
zouyx added a commit to zouyx/dubbo-go that referenced this pull request Sep 22, 2020
…ap[string]interface{} (apache#713)

* support attachment from map[sting]string to map[string]interface{} in invocation and result
# Conflicts:
#	cluster/router/tag/tag_router.go
AlexStocks pushed a commit that referenced this pull request Apr 14, 2021
…ap[string]interface{} (#713)

* support attachment from map[sting]string to map[string]interface{} in invocation and result
# Conflicts:
#	cluster/router/tag/tag_router.go
Malvin666 pushed a commit to Malvin666/dubbo-go that referenced this pull request Jun 8, 2021
…ap[string]interface{} (apache#713)

* support attachment from map[sting]string to map[string]interface{} in invocation and result
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants