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

provider can get attachment in ctx #508

Merged

Conversation

pantianying
Copy link
Member

What this PR does:

before this , consumer can send attachment from context
after this
Provider get attachment in context

@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #508 into develop will increase coverage by 0.04%.
The diff coverage is 25.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #508      +/-   ##
===========================================
+ Coverage    66.38%   66.43%   +0.04%     
===========================================
  Files          184      184              
  Lines         9717     9720       +3     
===========================================
+ Hits          6451     6457       +6     
+ Misses        2626     2625       -1     
+ Partials       640      638       -2     
Impacted Files Coverage Δ
common/proxy/proxy_factory/default.go 20.63% <0.00%> (-0.34%) ⬇️
common/proxy/proxy.go 91.26% <33.33%> (-1.81%) ⬇️
cluster/cluster_impl/base_cluster_invoker.go 62.31% <0.00%> (-10.15%) ⬇️
remoting/kubernetes/listener.go 56.07% <0.00%> (ø)
remoting/kubernetes/watch.go 79.24% <0.00%> (+0.94%) ⬆️
config_center/nacos/client.go 57.26% <0.00%> (+1.70%) ⬆️
cluster/cluster_impl/failback_cluster_invoker.go 80.64% <0.00%> (+2.15%) ⬆️
config_center/nacos/facade.go 62.50% <0.00%> (+25.00%) ⬆️

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 24e7205...d82e10f. Read the comment docs.

Copy link
Member

@flycash flycash left a comment

Choose a reason for hiding this comment

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

attachment本质上是key-value,如果只是把attachment里面的key-value拉平塞进去context,而不是直接塞整个attachment,这样是否可行?

@pantianying
Copy link
Member Author

attachment本质上是key-value,如果只是把attachment里面的key-value拉平塞进去context,而不是直接塞整个attachment,这样是否可行?

这里主要是参照java的模式 java中是通过context.getAttachment()获取到一个map[string]string ,而且客户端发送attachment之前已经确定格式了 现在也不好改了

@cvictory
Copy link
Contributor

cvictory commented May 9, 2020

晚点合并,等 #495 合并完成之后,再在纸上修改吧。

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.

result.SetAttachments(invocation.Attachments())

This line of code should be modified, otherwise modified attachment will not be returned.

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.

Still failing. boss Pan

@pantianying
Copy link
Member Author

result.SetAttachments(invocation.Attachments())

This line of code should be modified, otherwise modified attachment will not be returned.

现在暂不考虑服务端修改的ctx返回,因为ctx的传值机制是新生成ctx对象,只能向下传递,暂时无法向上传递修改内容

@AlexStocks AlexStocks merged commit 84b38fe into apache:develop May 15, 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.

7 participants