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

Fix service discovery related issues and add mesh proxy mode support #2022

Merged
merged 34 commits into from
Sep 29, 2022

Conversation

chickenlj
Copy link
Contributor

@chickenlj chickenlj commented Aug 18, 2022

  1. add 'mesh-enabled' property, 'true' means to deploy SDK together with sidecar
  2. make 'registry-type' to support 'service', 'interface', and 'all', representing register instance address, register interface address, and register both respectively. Default behavior is 'service'
  3. fix instance address notification issue

@chickenlj
Copy link
Contributor Author

Samples would be added to dubbo-go-samples soon

.asf.yaml Outdated Show resolved Hide resolved
.asf.yaml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2022

Codecov Report

Merging #2022 (3c7a964) into 3.1 (19ddecf) will decrease coverage by 0.02%.
The diff coverage is 40.62%.

❗ Current head 3c7a964 differs from pull request most recent head ea88557. Consider uploading reports for the commit ea88557 to get more accurate results

@@            Coverage Diff             @@
##              3.1    #2022      +/-   ##
==========================================
- Coverage   44.60%   44.58%   -0.03%     
==========================================
  Files         283      283              
  Lines       17105    17215     +110     
==========================================
+ Hits         7630     7675      +45     
- Misses       8669     8730      +61     
- Partials      806      810       +4     
Impacted Files Coverage Δ
common/extension/metadata_report_factory.go 0.00% <0.00%> (ø)
common/metadata_info.go 0.00% <0.00%> (ø)
config/config_loader.go 17.33% <0.00%> (-0.24%) ⬇️
config/consumer_config.go 22.04% <0.00%> (-0.54%) ⬇️
config/metadata_report_config.go 64.19% <0.00%> (ø)
config/reference_config.go 28.63% <0.00%> (-3.35%) ⬇️
config/service_config.go 52.25% <ø> (-0.80%) ⬇️
...y/event/service_instances_changed_listener_impl.go 0.00% <0.00%> (ø)
registry/service_instance.go 0.00% <0.00%> (ø)
...try/servicediscovery/service_discovery_registry.go 1.24% <0.00%> (-0.04%) ⬇️
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@chickenlj
Copy link
Contributor Author

apache/dubbo-go-samples#411

Merge this pull request first to pass the CI.

@Mulavar
Copy link
Member

Mulavar commented Aug 29, 2022

please squash your commits with git rebase.

@chickenlj
Copy link
Contributor Author

please squash your commits with git rebase.

Why is this necessary? I think 'squash and merge' would do the same thing for us if you worry too many commits would be brought in the repo after merging, it will compose all commits into one commit and the only thing that needs to do when using that is to replace the commit record listed there to one single conclusion.

@Mulavar
Copy link
Member

Mulavar commented Aug 29, 2022

the only thing that needs to do when using that is to replace the commit record listed there to one single conclusion.
yeah, that's what I want to mean. If you think this is not a problem, I am Ok with this.

common/metadata_info.go Outdated Show resolved Hide resolved
common/extension/metadata_report_factory.go Outdated Show resolved Hide resolved
config/registry_config.go Outdated Show resolved Hide resolved
config/registry_config.go Show resolved Hide resolved
@chickenlj
Copy link
Contributor Author

Kindly remind: please use 'Squash and Merge' when merging.

.asf.yaml Show resolved Hide resolved
ok = false
}

return r, ok
Copy link
Member

Choose a reason for hiding this comment

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

这个是不是多余了?直接判断是否为空字符串不就好了吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

return r, r == ""

@@ -515,6 +530,23 @@ func (c *URL) GetParam(s string, d string) string {
return r
}

// GetParamNoDefault gets value by key, return nil,false if no value found mapping to the key
func (c *URL) GetParamNoDefault(s string) (string, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这个函数名气很奇怪。在Go里面,我们知道map之类的会有一些default值,所以我大概也能明白你的用意。但 (string, bool) 中的 bool 就是 go 的风格,能够说明 key 是否存在,所以改名叫 GetParam 就可以了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

改名叫 GetParam 就可以

GetParam 这个名字已经被占用了,并且没有 bool 返回值,没啥好办法了。

Copy link
Member

Choose a reason for hiding this comment

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

You might consider using GetParamWithoutDefault to replace that.

Copy link

Choose a reason for hiding this comment

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

GetNonDefaultParam怎么样

Copy link
Contributor

Choose a reason for hiding this comment

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

GetNonDefaultParam怎么样

Agree with u. Really a better func name.

@Xezeloh
Copy link

Xezeloh commented Sep 27, 2022

GetNonDefaultParam

@AlexStocks AlexStocks merged commit 73efc8c into apache:3.1 Sep 29, 2022
@chickenlj chickenlj deleted the fix-3.0-mesh-proxy branch November 4, 2022 04:58
@chickenlj chickenlj restored the fix-3.0-mesh-proxy branch March 27, 2023 10:30
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.

8 participants