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

Config center feature implement for setting the zookeeper as config center to refresh consumerConfig & providerConfig when dubbogo starting. #99

Merged
merged 16 commits into from
Jun 29, 2019

Conversation

hxmhlt
Copy link
Contributor

@hxmhlt hxmhlt commented Jun 21, 2019

What this PR does:

New feature implement for config center, config refresh from config center when dubbogo start.
Now just have a zookeeper implement for config center.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
You can do a example in examples/dubbo/with-configcenter-go-server/

Does this PR introduce a user-facing change?:


@wongoo
Copy link
Contributor

wongoo commented Jun 22, 2019

how about make a conference to introduce it to reviewers when there is a big change like this?

@hxmhlt
Copy link
Contributor Author

hxmhlt commented Jun 22, 2019

1.Code in config folder have refactored for supporting config_center to refresh the consumerConfig or providerConfig when dubbogo starting. The core code is in base_config.go.
2.Interface in config_center defined and I implement zookeeper as config_center ,also make it compatible with java 2.7.2.
3.TODO: Dynamic config by using Listeners's in dynamicConfigCenter. Maybe next pr.

@hxmhlt
Copy link
Contributor Author

hxmhlt commented Jun 22, 2019

how about make a conference to introduce it to reviewers when there is a big change like this?

I write the main work in this pr above . May can help you review the code.

@codecov-io
Copy link

codecov-io commented Jun 22, 2019

Codecov Report

Merging #99 into master will increase coverage by 4.16%.
The diff coverage is 55.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   63.11%   67.27%   +4.16%     
==========================================
  Files          54       67      +13     
  Lines        3652     4086     +434     
==========================================
+ Hits         2305     2749     +444     
+ Misses       1120     1052      -68     
- Partials      227      285      +58
Impacted Files Coverage Δ
config_center/dynamic_configuration.go 0% <ø> (ø)
config_center/mock_dynamic_config.go 0% <0%> (ø)
common/url.go 66.47% <100%> (+0.98%) ⬆️
config/reference_config.go 91.78% <100%> (+0.23%) ⬆️
remoting/zookeeper/facade.go 74.28% <100%> (ø)
config/service_config.go 68.49% <100%> (+0.88%) ⬆️
config/protocol_config.go 100% <100%> (ø)
registry/directory/directory.go 78.3% <100%> (ø) ⬆️
remoting/zookeeper/client.go 59.53% <25%> (+33.66%) ⬆️
remoting/zookeeper/listener.go 43.78% <37.77%> (+43.78%) ⬆️
... and 31 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 d1d8715...d9a170d. Read the comment docs.

common/config/environment.go Show resolved Hide resolved
common/config/environment.go Show resolved Hide resolved
common/constant/default.go Outdated Show resolved Hide resolved
common/extension/config_center_factory.go Outdated Show resolved Hide resolved
config/base_config.go Outdated Show resolved Hide resolved
config_center/dynamic_configuration_factory.go Outdated Show resolved Hide resolved
config_center/mock_dynamic_config.go Outdated Show resolved Hide resolved
config_center/mock_dynamic_config.go Outdated Show resolved Hide resolved
protocol/jsonrpc/http.go Outdated Show resolved Hide resolved
remoting/listener.go Outdated Show resolved Hide resolved
config/protocol_config.go Show resolved Hide resolved
config/method_config.go Outdated Show resolved Hide resolved
config/provider_config.go Outdated Show resolved Hide resolved
config_center/configuration_parser.go Outdated Show resolved Hide resolved
config_center/zookeeper/impl.go Outdated Show resolved Hide resolved
config_center/zookeeper/impl.go Outdated Show resolved Hide resolved
config_center/zookeeper/impl_test.go Show resolved Hide resolved
if conLen == 0 {
panic("conMap is nil")
}
config.Load()
Copy link
Contributor

Choose a reason for hiding this comment

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

why delete check condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

config.Load方法已经改为无返回值。之前是返回加载成功的配置数,与yincheng讨论后觉得没有必要,且不优雅。

- id: "hangzhouzk"
type: "zookeeper"
"hangzhouzk":
protocol: "zookeeper"
Copy link
Contributor

Choose a reason for hiding this comment

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

type -> protocol? Is zookeeper a protocol?

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 Author

Choose a reason for hiding this comment

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

是的,这里与java的叫法保持一致了。

@@ -30,7 +30,7 @@ import (
"github.com/apache/dubbo-go/common/logger"
)

type ZkClientContainer interface {
type zkClientFacade interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

which change its name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

container叫法感觉别扭,感觉这种实现方式,类似于设计模式之门面模式。

return instance
}

//func (env *Environment) SetConfigCenterFirst() {

Choose a reason for hiding this comment

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

Why not remove comment?

// env.configCenterFirst = true
//}

//func (env *Environment) ConfigCenterFirst() bool {

Choose a reason for hiding this comment

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

As above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里我估计后面添加新的配置级别,就会要加上,所以没有直接去掉,先注释掉了。先不删吧

config_center/zookeeper/impl_test.go Show resolved Hide resolved
config_center/zookeeper/impl_test.go Show resolved Hide resolved
config/provider_config.go Outdated Show resolved Hide resolved
config/provider_config.go Show resolved Hide resolved
common/url.go Outdated Show resolved Hide resolved
@gaoxinge
Copy link

I have added some review comments on codes. I also want to ask when this pr will be merged into master?

@AlexStocks
Copy link
Contributor

I have added some review comments on codes. I also want to ask when this pr will be merged into master?

I do not know. So big pr.

@AlexStocks AlexStocks merged commit 1aa5bcb into apache:master Jun 29, 2019
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