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

feat(registry/zk):url encode zkpath #283

Merged
merged 1 commit into from
Dec 19, 2019

Conversation

xujianhai666
Copy link
Member

What this PR does:
be consistent with java impl, support url encoder for zkpath

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.

I had checked codes using the zkPath. It seems that we don't need to decode the zkPath when we get the zkPath. Right?

@xujianhai666
Copy link
Member Author

I had checked codes using the zkPath. It seems that we don't need to decode the zkPath when we get the zkPath. Right?

@flycash yes, just for zk registry

@codecov-io
Copy link

Codecov Report

Merging #283 into develop will increase coverage by 0.81%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #283      +/-   ##
===========================================
+ Coverage    65.52%   66.34%   +0.81%     
===========================================
  Files          111      111              
  Lines         7063     7065       +2     
===========================================
+ Hits          4628     4687      +59     
+ Misses        1978     1919      -59     
- Partials       457      459       +2
Impacted Files Coverage Δ
registry/zookeeper/registry.go 55.39% <100%> (+0.32%) ⬆️
cluster/cluster_impl/failback_cluster_invoker.go 80.64% <0%> (+2.15%) ⬆️
protocol/dubbo/readwriter.go 70.37% <0%> (+2.46%) ⬆️
config_center/apollo/impl.go 87.01% <0%> (+2.59%) ⬆️
protocol/dubbo/codec.go 82.35% <0%> (+5.88%) ⬆️
protocol/dubbo/listener.go 61.9% <0%> (+10.71%) ⬆️
protocol/dubbo/pool.go 73.6% <0%> (+15.73%) ⬆️

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 d65659f...c0839f0. Read the comment docs.

@zouyx
Copy link
Member

zouyx commented Dec 18, 2019

I had checked codes using the zkPath too. we use zkPath to registry and get children node,so it seems that no need to decode.

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

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

@hxmhlt
Copy link
Contributor

hxmhlt commented Dec 19, 2019

I checked java version. There is method named 'encoded' in URL.java and it called by ZookeeperRegistry for zkPath encode. Though I have not received any issue about this problem...Maybe it is encoded by zk golang client.

@hxmhlt hxmhlt merged commit 5a50da7 into apache:develop Dec 19, 2019
@xujianhai666
Copy link
Member Author

I had checked codes using the zkPath too. we use zkPath to registry and get children node,so it seems that no need to decode.

@zouyx for consistent with java impl. Besides, grpc need this decode method to resolve "$" for inner class

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.

6 participants