Feature/polaris feat: Add Polaris support #1797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1797 +/- ##
==========================================
- Coverage 73.46% 73.25% -0.22%
==========================================
Files 495 503 +8
Lines 44328 45392 +1064
==========================================
+ Hits 32566 33252 +686
- Misses 9801 10115 +314
- Partials 1961 2025 +64
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
houseme
changed the title
gqcn
changed the title
gqcn
requested changes
May 13, 2022
net/ghttp/ghttp_server_registry.go
Outdated
| @@ -47,7 +47,7 @@ func (s *Server) doServiceRegister() { | |||
| } | |||
| s.service = &gsvc.Service{ | |||
| Name: s.GetName(), | |||
| Endpoints: []string{fmt.Sprintf(`%s:%s`, ip, port)}, | |||
| Endpoints: []string{fmt.Sprintf(`%s://%s:%s`, protocol, ip, port)}, | |||
There was a problem hiding this comment.
Endpoints的格式统一采用ip:port的格式，至于使用端如何使用，按照此格式解析。不要随便修改这个格式。
net/gsvc/gsvc_service.go
Outdated
| // Separator is the default defaultSeparator for path. | ||
| func Separator() string { | ||
| return defaultSeparator | ||
| } |
There was a problem hiding this comment.
如果defaultSeparator需要被外部访问，定义为公开常量即可，不用再定义个方法来暴露。
net/gsvc/gsvc_service.go
Outdated
| if s.Separator != defaultSeparator { | ||
| return gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, s.Separator) | ||
| } | ||
| return s.Separator + gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, s.Separator) |
There was a problem hiding this comment.
请参考如下：
var separator = defaultSeparator
if s.Separator != "" {
separator = s.Separator
}
return separator + gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, separator)
net/gsvc/gsvc_service.go
Outdated
| if s.Separator != defaultSeparator { | ||
| return gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, s.Separator) | ||
| } | ||
| return s.Separator + gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, s.Separator) |
There was a problem hiding this comment.
请参考如下：
var separator = defaultSeparator
if s.Separator != "" {
separator = s.Separator
}
return separator + gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, separator)
net/gsvc/gsvc_service.go
Outdated
| @@ -19,47 +19,60 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| separator = "/" | |||
| defaultSeparator = "/" | |||
| delimiter = "," | |||
There was a problem hiding this comment.
没有必要单独定义一个常量。如果你实在是想要定义，可以定义为endpointDelimiter。
contrib/registry/polaris/polaris.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| _ gsvc.Registrar = &Registry{} |
There was a problem hiding this comment.
这里应该是_ gsvc.Registry = &Registry{}，该断言保障Registry对象能同时实现注册和发现两个特性，参考下etcd实现。
contrib/registry/polaris/polaris.go
Outdated
| ) | ||
|
|
||
| // _instanceIDSeparator Instance id Separator. | ||
| const _instanceIDSeparator = "-" |
# Conflicts: # example/go.sum # go.sum
gqcn
requested changes
May 16, 2022
contrib/registry/polaris/polaris.go
Outdated
| } | ||
|
|
||
| // NewRegistry create a new registry. | ||
| func NewRegistry(provider polaris.ProviderAPI, consumer polaris.ConsumerAPI, opts ...Option) (r *Registry) { |
contrib/registry/polaris/polaris.go
Outdated
| } | ||
|
|
||
| // NewRegistryWithConfig new a registry with config. | ||
| func NewRegistryWithConfig(conf config.Configuration, opts ...Option) (r *Registry) { |
There was a problem hiding this comment.
NewRegistryWithConfig -> NewWithConfig
contrib/registry/polaris/polaris.go
Outdated
|
|
||
| // getHostAndPortFromEndpoint get host and port from endpoint. | ||
| func getHostAndPortFromEndpoint(ctx context.Context, endpoint string) (host string, port int, err error) { | ||
| endpoint = gstr.ReplaceByArray(endpoint, []string{"tcp://", "", "udp://", "", "http://", "", "https://", "", "ws://", "", "wss://", ""}) |
There was a problem hiding this comment.
不用做替换，使用默认的endpoint约定，即host:port格式。
这个格式倒是可以在gsvc那边服务注册的时候加个校验，而不是在所有使用的地方再去校验。
contrib/registry/polaris/polaris.go
Outdated
| // getHostAndPortFromEndpoint get host and port from endpoint. | ||
| func getHostAndPortFromEndpoint(ctx context.Context, endpoint string) (host string, port int, err error) { | ||
| endpoint = gstr.ReplaceByArray(endpoint, []string{"tcp://", "", "udp://", "", "http://", "", "https://", "", "ws://", "", "wss://", ""}) | ||
| httpArr := gstr.Split(endpoint, endpointDelimiter) |
There was a problem hiding this comment.
- 为什么用
httpArr，这是HTTP地址吗？ gstr.Split -> gstr.SplitAndTrim
net/gsvc/gsvc_service.go
Outdated
| if s.Separator != DefaultSeparator { | ||
| return gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, defaultSeparator) | ||
| } | ||
| return defaultSeparator + gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, defaultSeparator) |
There was a problem hiding this comment.
这段代码重复率标高，请参考如下：
var separator = defaultSeparator
if s.Separator != "" {
separator = s.Separator
}
return separator + gstr.Join([]string{s.Prefix, s.Deployment, s.Namespace, s.Name, s.Version}, separator)There was a problem hiding this comment.
这个地方是和默认的不相同，在最前面不添加分隔符。
gqcn
requested changes
May 16, 2022
contrib/registry/polaris/polaris.go
Outdated
| func getHostAndPortFromEndpoint(ctx context.Context, endpoint string) (host string, port int, err error) { | ||
| endpoints := gstr.SplitAndTrim(endpoint, endpointDelimiter) | ||
| if len(endpoints) < 2 { | ||
| err = gerror.New("invalid endpoint") |
There was a problem hiding this comment.
err = gerror.Newf(`invalid endpoint "%s"`, endpoint)
就一个invalid endpoint不足以反馈问题，报错的时候调用端不知道是什么endpoint引起的报错。
chuntaojun
reviewed
May 17, 2022
|
|
||
| if r.opt.Heartbeat { | ||
| // start heartbeat report | ||
| go func() { |
There was a problem hiding this comment.
是不是需要进行控制下这个协程？反注册的话走context.Cancel进行取消
gqcn
approved these changes
May 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Polaris service
#1504
#1826
polarismesh/polaris#163