Skip to content
New issue

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

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

Already on GitHub? # to your account

Mod:refactor to add base_registry #348

Merged
merged 7 commits into from
Feb 7, 2020
Merged

Conversation

hxmhlt
Copy link
Contributor

@hxmhlt hxmhlt commented Feb 3, 2020

What this PR does:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@codecov-io
Copy link

codecov-io commented Feb 3, 2020

Codecov Report

Merging #348 into develop will decrease coverage by 0.5%.
The diff coverage is 89.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #348      +/-   ##
===========================================
- Coverage    67.65%   67.14%   -0.51%     
===========================================
  Files          123      145      +22     
  Lines         7317     7692     +375     
===========================================
+ Hits          4950     5165     +215     
- Misses        1894     2037     +143     
- Partials       473      490      +17
Impacted Files Coverage Δ
config/consumer_config.go 51.92% <ø> (-4.22%) ⬇️
config_center/zookeeper/impl.go 40% <ø> (ø) ⬆️
remoting/zookeeper/listener.go 52.02% <ø> (ø) ⬆️
registry/zookeeper/listener.go 60.86% <ø> (ø) ⬆️
config/provider_config.go 52.77% <ø> (-5.76%) ⬇️
registry/etcdv3/listener.go 76.74% <ø> (ø) ⬆️
config/base_config.go 63.95% <ø> (ø) ⬆️
config/metric_config.go 0% <0%> (ø)
remoting/zookeeper/client.go 68.06% <100%> (ø) ⬆️
metrics/prometheus/reporter.go 100% <100%> (ø)
... and 30 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 af4683c...0703ced. 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.

Did you test it end to end? Or run the samples?

@pantianying
Copy link
Member

Can you remove the recursive logic from the zookeeper registry center in this PR,distinguish from configuration center😛😘

@hxmhlt
Copy link
Contributor Author

hxmhlt commented Feb 5, 2020

Did you test it end to end? Or run the samples?

Yes,I run it in dubbo-samples

@hxmhlt
Copy link
Contributor Author

hxmhlt commented Feb 5, 2020

Can you remove the recursive logic from the zookeeper registry center in this PR,distinguish from configuration center😛😘

I think it will be another pr. We need to create a new pull request to do that.

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

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.

LGTM, but need some comment for registry/base_registry.go

registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
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

registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/base_registry.go Show resolved Hide resolved
registry/etcdv3/listener.go Outdated Show resolved Hide resolved
@AlexStocks
Copy link
Contributor

LGTM

@AlexStocks AlexStocks merged commit ce82969 into apache:develop Feb 7, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants