-
Notifications
You must be signed in to change notification settings - Fork 929
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
Istio/XDS support #1804
Istio/XDS support #1804
Conversation
xds circuit breaker max request
* fix: ut * Fix: add license
.github/workflows/github-actions.yml
Outdated
@@ -14,7 +14,7 @@ jobs: | |||
# If you want to matrix build , you can append the following list. | |||
matrix: | |||
go_version: | |||
- 1.15 | |||
- 1.17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我们要升级这个版本吗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我认为升级吧。
1.15 版本不支持一些基础函数,比如os.ReadFile
开发者本地如果使用高版本,ci的时候会报错
@zhaoyunxing92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed yesterday night, pls recover the go version.
Codecov Report
@@ Coverage Diff @@
## 3.0 #1804 +/- ##
==========================================
- Coverage 47.17% 46.76% -0.41%
==========================================
Files 298 298
Lines 17401 17157 -244
==========================================
- Hits 8209 8024 -185
+ Misses 8339 8288 -51
+ Partials 853 845 -8
Continue to review full report at Codecov.
|
ctx := invocation.ToContext() | ||
matcher, err := resource.RouteToMatcher(r) | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic 不能随便用,这里确实有必要使用 panic 吗?
go.mod
Outdated
@@ -1,6 +1,6 @@ | |||
module dubbo.apache.org/dubbo-go/v3 | |||
|
|||
go 1.15 | |||
go 1.17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
欢迎 go 版本。
go.mod
Outdated
google.golang.org/grpc v1.45.0 | ||
google.golang.org/protobuf v1.28.0 | ||
gopkg.in/yaml.v2 v2.4.0 | ||
k8s.io/apimachinery v0.22.4 | ||
k8s.io/client-go v0.16.9 | ||
) | ||
|
||
require ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
加这么多 require 块作甚?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
合并成一个吧。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个是go 1.17的特性,切回去go版本就好了。
protocol/invocation.go
Outdated
@@ -57,4 +58,6 @@ type Invocation interface { | |||
SetAttribute(key string, value interface{}) | |||
GetAttribute(key string) (interface{}, bool) | |||
GetAttributeWithDefaultValue(key string, defaultValue interface{}) interface{} | |||
|
|||
ToContext() context.Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetContext() may be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to 'GetAttachmentAsContext', which is more clear.
registry/xds/registry.go
Outdated
} | ||
|
||
func isProvider(url *common.URL) bool { | ||
return getCategory(url) == "providers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"providers" 在 如下地方有定义:
./common/constant/default.go:77: ProviderCategory = "providers"
不要在这里直接使用字符串。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
remoting/xds/common/model.go
Outdated
"strings" | ||
) | ||
|
||
type Addr struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我反对你重新封装这么一个 struct, net.Addr 不能用吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
官方库的net.Addr只是一个接口,并不是具体类型。
第三房库也有类似的实现,我认为不如自己定义一个用的方便和清晰。 @AlexStocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 继承 Addr 接口
2 看是否有必要放入 common
remoting/xds/common/model.go
Outdated
Port string | ||
} | ||
|
||
func NewAddr(addr string) Addr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net. 包内部有 net.SplitHostPort
remoting/xds/common/model.go
Outdated
} | ||
} | ||
|
||
func (a *Addr) String() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net 包内部有 net.JoinHostPort
从上面分析来看,你没必要封装这么一个东西。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我认为还是需要封装一个自己的Addr,用来以字符串形式存储“地址 + 端口”
官方提供的net.TCPAddr, 其ip必须是真实的ip,并不是字符串封装的地址例如"istiod.istio-system.svc.cluster.local",不能在我的场景使用。
上面几个评论,使用官方库来进行地址和ip+port的转换,这个我会修复。
但我还是希望保留这个Addr 对象。 @AlexStocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那你把名字清晰化,譬如取名 IstioXDSAddr 之类的
* limitations under the License. | ||
*/ | ||
|
||
package interfaceMapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interfaceMapping 这个 package name 是从 xds 里面搞过来的吗?如果不是,直接改用 mapping 就可以了。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
func (i *InterfaceMapHandlerImpl) GetHostAddrMap(serviceUniqueKey string) (string, error) { | ||
i.interfaceNameHostAddrMapLock.RLock() | ||
if hostAddr, ok := i.interfaceNameHostAddrMap[serviceUniqueKey]; ok { | ||
return hostAddr, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG,deadlock
if str, ok := v.([]string); ok { | ||
gRPCMD.Set(k, str...) | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里如果是其他类型,是不是得几个错误 log?
registry/directory/directory.go
Outdated
@@ -312,9 +314,28 @@ func (dir *RegistryDirectory) toGroupInvokers() []protocol.Invoker { | |||
return groupInvokersList | |||
} | |||
|
|||
func (dir *RegistryDirectory) uncacheInvokerWithClusterId(clusterId string) []protocol.Invoker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClusterId --> ClusterID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
函数名称和变量名称都改下
* | ||
*/ | ||
|
||
package clusterresolver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clusterresolver 这里的 package 和 路径名字改为 resolver 如何?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codes in xds/* are refered form grpc @AlexStocks
|
||
"google.golang.org/grpc/resolver" | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下面这个 struct 还有本文件名称命名很奇怪,Resolve 是个动词。文件名称里面带有 now,这很奇怪。你是要表达什么意思?详细说下,我给你换个名字。
What this PR does:
Added Istio/XDS support
We Refered xdsClient impl of gRPC, and retained copy right of gRPC developers.
增加了Istio/XDS 支持
我们引用了gRPC的xdsClient impl,并保留了gRPC开发者的版权。
Which issue(s) this PR fixes:
Fixes #1803
To Developers
You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed