You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by GitBox <gi...@apache.org> on 2022/01/06 07:21:56 UTC
[GitHub] [servicecomb-service-center] xiaoluoluo opened a new pull request #1205: [feat]add health api in sync package
xiaoluoluo opened a new pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205
Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SCB) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[SCB-XXX] Fixes bug in ApproximateQuantiles`, where you replace `SCB-XXX` with the appropriate JIRA issue.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `go build` `go test` `go fmt` `go vet` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
- [ ] Never comment source code, delete it.
- [ ] UT should has "context, subject, expected result" result as test case name, when you call t.Run().
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780086179
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
Review comment:
不必维护分支，都是resolver，之所以可以代码合一是因为用例就一个：集群
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui merged pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui merged pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779984022
##########
File path: syncer/resource/admin/admin.go
##########
@@ -34,6 +35,6 @@ func (res *Resource) URLPatterns() []rest.Route {
}
func (res *Resource) HealthCheck(w http.ResponseWriter, r *http.Request) {
Review comment:
需要吧这个接口注册到白名单，rbac等认证场景跳过校验
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780087991
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
+ for _, a := range c.Addr {
+ addr = append(addr, resolver.Address{Addr: a})
+ }
+ r := manual.NewBuilderWithScheme(c.Scheme)
+ r.InitialState(resolver.State{Addresses: addr})
+ conn, err = grpc.Dial(r.Scheme()+":///", grpc.WithInsecure(), grpc.WithResolvers(r))
Review comment:
为什么有3个/ 给下标准的依据
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780621652
##########
File path: server/rpc/sync/server.go
##########
@@ -16,3 +22,12 @@ func (s *Server) Sync(ctx context.Context, events *v1sync.EventList) (*v1sync.Re
log.Info(fmt.Sprintf("Received: %v", events.Events[0].Action))
return &v1sync.Results{}, nil
}
+
+func (s *Server) Health(ctx context.Context, request *v1sync.HealthRequest) (*v1sync.HealthReply, error) {
+ log.Info(fmt.Sprintf("Health Received"))
Review comment:
这边如果不打印变量的话可以直接"Health Received"，不需要fmt.Sprintf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang edited a comment on pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang edited a comment on pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#issuecomment-1006501487
> #1206 add health api
PR描述给出有价值的信息，是否有issue跟踪反而不是我关心的，参考下我的提交
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#issuecomment-1007871646
golangci-lint / lint (pull_request) 规范问题解决一下 之前看一只没过
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#issuecomment-1007871646
golangci-lint / lint (pull_request) 规范问题解决一下 之前看一只没过
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779987452
##########
File path: syncer/health/health.go
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "context"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+)
+
+type Resp struct {
+ DataCenter *DataCenter `json:"datacenter"`
+ Peers []*Peer `json:"peer"`
Review comment:
叫peers
##########
File path: syncer/service/admin/health.go
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package admin
+
+import (
+ "context"
+ "errors"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/pkg/rpc"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+ serviceName = "service-center"
Review comment:
应该叫“syncer”
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780643270
##########
File path: server/rpc/sync/server.go
##########
@@ -16,3 +23,12 @@ func (s *Server) Sync(ctx context.Context, events *v1sync.EventList) (*v1sync.Re
log.Info(fmt.Sprintf("Received: %v", events.Events[0].Action))
return &v1sync.Results{}, nil
}
+
+func (s *Server) Health(ctx context.Context, request *v1sync.HealthRequest) (*v1sync.HealthReply, error) {
+ log.Info("Health Received")
Review comment:
可以不打日志
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779351708
##########
File path: api/sync/v1/event_service.proto
##########
@@ -18,6 +18,14 @@ message Result {
int32 code = 1; //reuse standard http code
string message = 2;
}
+
+message HealthRequest {
+}
+message HealthReply {
+ string status = 1;
Review comment:
detail message在下一个迭代？
##########
File path: client/set.go
##########
@@ -29,3 +30,21 @@ func NewSetForConfig(c SetConfig) (*Set, error) {
EventServiceClient: v1sync.NewEventServiceClient(conn),
}, nil
}
+
+// NewSetForAddrList dial grpc connection and create all grpc clients
+func NewSetForAddrList(scheme string, addrList []string) (*Set, error) {
Review comment:
不要新增方法，而是在SetConfig里面加新参数
##########
File path: syncer/health/health_test.go
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "testing"
+
+ "github.com/apache/servicecomb-service-center/syncer/config"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestHealth(t *testing.T) {
+ assert.NoError(t, config.Init())
Review comment:
还需要提供2个test case 一个是非健康的，一个是健康的
##########
File path: syncer/config/config.go
##########
@@ -33,11 +33,24 @@ type Config struct {
}
type Sync struct {
- Peers []*Peer `yaml:"peers"`
+ EnableOnStart bool `yaml:"enableOnStart"`
+ DataCenter *DataCenter `yaml:"datacenter"`
+ Peers []*Peer `yaml:"peers"`
+}
+
+type DataCenter struct {
+ Name string `yaml:"name"`
+ Region string `yaml:"region"`
+ AvailableZone string `yaml:"availableZone"`
}
type Peer struct {
- // TODO
+ Name string `yaml:"name"`
+ Kind string `yaml:"kind"`
+ Endpoints []string `yaml:"endpoints"`
+ Mode []string `yaml:"mode"`
+ CaFile string `yaml:"caFile"`
+ Revision string `yaml:"revision"`
Review comment:
Revision是什么
##########
File path: syncer/health/health_test.go
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "testing"
+
+ "github.com/apache/servicecomb-service-center/syncer/config"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestHealth(t *testing.T) {
+ assert.NoError(t, config.Init())
Review comment:
UT需要有test case name
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui merged pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui merged pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779998472
##########
File path: server/rpc/sync/server.go
##########
@@ -3,9 +3,15 @@ package sync
import (
"context"
"fmt"
-
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
+ "github.com/apache/servicecomb-service-center/server/config"
+)
+
+const (
+ HealthConnected = "CONNECTED"
Review comment:
StatusConnected会不会更好一点
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780086179
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
Review comment:
不必维护分支，都是resolver，之所以可以代码合一是因为用例就一个：拨集群
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] xiaoluoluo commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
xiaoluoluo commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780154763
##########
File path: client/set.go
##########
@@ -1,16 +1,25 @@
package client
import (
+ "errors"
"fmt"
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
"google.golang.org/grpc"
+ "google.golang.org/grpc/resolver"
+ "google.golang.org/grpc/resolver/manual"
+)
+
+var (
+ ErrAddrEmpty = errors.New("addr is empty")
+ ErrConnNil = errors.New("conn is nil")
)
// SetConfig is client configs
type SetConfig struct {
- Addr string
+ Addr []string
+ Scheme string
Review comment:
调用请求的时候 会 注册上 解析器 schema 是解析器的名字，这里可以根据自己的业务自己定义
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780089309
##########
File path: etc/conf/app.yaml
##########
@@ -144,6 +144,10 @@ registry:
# the allowable minimum value of instance heartbeat interval
# if interval < minInterval, instance TTL still set with minInterval
minInterval: 5s
+ datacenter:
Review comment:
这数据是否和sync路径下的配置冗余了
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] xiaoluoluo commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
xiaoluoluo commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780155235
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
+ for _, a := range c.Addr {
+ addr = append(addr, resolver.Address{Addr: a})
+ }
+ r := manual.NewBuilderWithScheme(c.Scheme)
+ r.InitialState(resolver.State{Addresses: addr})
+ conn, err = grpc.Dial(r.Scheme()+":///", grpc.WithInsecure(), grpc.WithResolvers(r))
Review comment:
这里在请求的时候 grpc.WithResolvers(r)) 带上解析器
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780643397
##########
File path: syncer/service/admin/health.go
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package admin
+
+import (
+ "context"
+ "errors"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/pkg/rpc"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+ serviceName = "service-center"
+)
+
+var (
+ ErrConfigIsEmpty = errors.New("sync config is empty")
+)
+
+type Resp struct {
+ Peers []*Peer `json:"peers"`
+}
+
+type Peer struct {
+ Name string `json:"name"`
+ Kind string `json:"kind"`
+ Mode []string `json:"mode"`
+ Endpoints []string `json:"endpoints"`
+ Status string `json:"status"`
+}
+
+func Health() (*Resp, error) {
+
+ config := config.GetConfig()
+ if config.Sync == nil || len(config.Sync.Peers) <= 0 {
+ return nil, ErrConfigIsEmpty
+ }
+
+ resp := &Resp{Peers: make([]*Peer, 0, len(config.Sync.Peers))}
+
+ for _, c := range config.Sync.Peers {
+ if len(c.Endpoints) <= 0 {
+ continue
+ }
+ p := &Peer{
+ Name: c.Name,
+ Kind: c.Kind,
+ Mode: c.Mode,
+ Endpoints: c.Endpoints,
+ }
+ p.Status = getPeerStatus(c.Endpoints)
+ resp.Peers = append(resp.Peers, p)
+ }
+
+ if len(resp.Peers) <= 0 {
+ return nil, ErrConfigIsEmpty
+ }
+
+ return resp, nil
+}
+
+func getPeerStatus(endpoints []string) string {
+ conn, err := rpc.GetRoundRobinLbConn(&rpc.Config{Addrs: endpoints, Scheme: scheme, ServiceName: serviceName})
+ if err != nil || conn == nil {
+ return sync.HealthStatusAbnormal
+ }
+ defer conn.Close()
+
+ set := client.NewSetForConfig(conn)
Review comment:
改名：client.NewSet(conn)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780621502
##########
File path: client/set.go
##########
@@ -19,13 +32,29 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
Review comment:
三个返回值是不是不太好，一般是2个，华哥怎么看
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] xiaoluoluo commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
xiaoluoluo commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779470711
##########
File path: api/sync/v1/event_service.proto
##########
@@ -18,6 +18,14 @@ message Result {
int32 code = 1; //reuse standard http code
string message = 2;
}
+
+message HealthRequest {
+}
+message HealthReply {
+ string status = 1;
Review comment:
暂时不需要 detail 字段
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#issuecomment-1006341160
PR描述写一下
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779963509
##########
File path: syncer/config/config.go
##########
@@ -33,11 +33,22 @@ type Config struct {
}
type Sync struct {
- Peers []*Peer `yaml:"peers"`
+ EnableOnStart bool `yaml:"enableOnStart"`
+ DataCenter *DataCenter `yaml:"datacenter"`
Review comment:
不应该有此属性
##########
File path: syncer/health/health.go
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "context"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+)
+
+type Resp struct {
+ DataCenter *DataCenter `json:"datacenter"`
+ Peers []*Peer `json:"peer"`
+}
+
+type DataCenter struct {
+ Name string `json:"name"`
+}
+
+type Peer struct {
+ Name string `json:"name"`
+ Kind string `json:"kind"`
+ Mode []string `json:"mode"`
+ Endpoints []string `json:"endpoints"`
+ Status string `json:"status"`
+}
+
+func Health() *Resp {
Review comment:
应该有error返回，用于外层判断是否内部错误异常
##########
File path: server/rpc/sync/server.go
##########
@@ -16,3 +22,12 @@ func (s *Server) Sync(ctx context.Context, events *v1sync.EventList) (*v1sync.Re
log.Info(fmt.Sprintf("Received: %v", events.Events[0].Action))
return &v1sync.Results{}, nil
}
+
+func (s *Server) Health(ctx context.Context, request *v1sync.HealthRequest) (*v1sync.HealthReply, error) {
+ log.Info(fmt.Sprintf("Health Received"))
+ syncerEnabled := config.GetBool("syncer.enabled", false)
Review comment:
配置项不正确，应该是sync.enableOnStart
##########
File path: syncer/health/health.go
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "context"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+)
+
+type Resp struct {
+ DataCenter *DataCenter `json:"datacenter"`
Review comment:
没有此属性
##########
File path: client/set.go
##########
@@ -20,11 +24,33 @@ type Set struct {
// NewSetForConfig dial grpc connection and create all grpc clients
func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+ if len(c.Addr) <= 0 {
+ return nil, errors.New("addr is empty")
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
+ for _, a := range c.Addr {
+ addr = append(addr, resolver.Address{Addr: a})
+ }
+ r := manual.NewBuilderWithScheme(c.Scheme)
+ r.InitialState(resolver.State{Addresses: addr})
+ conn, err = grpc.Dial(r.Scheme()+":///", grpc.WithInsecure(), grpc.WithResolvers(r))
Review comment:
安全问题：链接泄露，没有close
##########
File path: syncer/health/health.go
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "context"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+)
+
+type Resp struct {
+ DataCenter *DataCenter `json:"datacenter"`
+ Peers []*Peer `json:"peer"`
+}
+
+type DataCenter struct {
+ Name string `json:"name"`
+}
+
+type Peer struct {
+ Name string `json:"name"`
+ Kind string `json:"kind"`
+ Mode []string `json:"mode"`
+ Endpoints []string `json:"endpoints"`
+ Status string `json:"status"`
+}
+
+func Health() *Resp {
+ resp := &Resp{
+ DataCenter: &DataCenter{},
+ Peers: make([]*Peer, 0),
+ }
+
+ config := config.GetConfig()
+ if config.Sync == nil || config.Sync.DataCenter == nil {
+ return resp
+ }
+
+ resp.DataCenter.Name = config.Sync.DataCenter.Name
+ for _, c := range config.Sync.Peers {
+ if len(c.Endpoints) <= 0 {
+ continue
+ }
+ p := &Peer{
+ Name: c.Name,
+ Kind: c.Kind,
+ Mode: c.Mode,
+ Endpoints: c.Endpoints,
+ }
+ p.Status = getPeerStatus(c.Endpoints)
+ resp.Peers = append(resp.Peers, p)
+ }
+ return resp
+}
+
+func getPeerStatus(endpoints []string) string {
+ client, err := client.NewSetForConfig(client.SetConfig{Addr: endpoints, Scheme: scheme})
Review comment:
未有fallback策略，3次重试，单次超时10s，参考：https://github.com/grpc/grpc-go/blob/master/examples/features/retry/client/main.go
##########
File path: syncer/health/health.go
##########
@@ -0,0 +1,89 @@
+/*
Review comment:
应该放到syncer/service/admin/health.go下
##########
File path: syncer/health/health_test.go
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "testing"
+
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+func TestHealth(t *testing.T) {
+ c := config.GetConfig()
+ tests := []struct {
+ name string
+ sync *config.Sync
+ wantErr bool
+ }{
+ {name: "check no config ",
+ sync: nil,
+ wantErr: true,
+ },
+ {name: "check no dataCenter",
+ sync: &config.Sync{
+ DataCenter: nil,
+ Peers: []*config.Peer{},
+ },
+ wantErr: true,
+ },
+ {name: "check no endpoints",
+ sync: &config.Sync{
+ DataCenter: &config.DataCenter{Name: "tets"},
+ Peers: []*config.Peer{
+ {Endpoints: nil},
+ },
+ },
+ wantErr: true,
+ },
+ {name: "check endpoints is empty",
+ sync: &config.Sync{
+ DataCenter: &config.DataCenter{Name: "tets"},
+ Peers: []*config.Peer{
+ {Endpoints: []string{}},
+ },
+ },
+ wantErr: true,
+ },
+
+ {name: "given normal config",
+ sync: &config.Sync{
+ DataCenter: &config.DataCenter{Name: "tets"},
+ Peers: []*config.Peer{
+ {Endpoints: []string{"127.0.0.1:30105"}},
+ },
+ },
+ wantErr: false,
+ },
+ }
+
+ for _, test := range tests {
+ c.Sync = test.sync
+ config.SetConfig(c)
+ resp := Health()
+
+ hasErr := false
+ if resp.DataCenter == nil {
+ hasErr = true
+ }
+
+ if resp.Peers == nil {
+ hasErr = true
+ }
+
+ if len(resp.Peers) <= 0 {
+ hasErr = true
+ }
+
+ if hasErr != test.wantErr {
+ t.Errorf("%s. Health(), wantErr %+v", test.name, test.wantErr)
+ }
Review comment:
1、UT用assert断言
2、缺少正常场景的验证
##########
File path: syncer/resource/admin/admin.go
##########
@@ -34,6 +35,6 @@ func (res *Resource) URLPatterns() []rest.Route {
}
func (res *Resource) HealthCheck(w http.ResponseWriter, r *http.Request) {
- // TODO call health service
- rest.WriteResponse(w, r, nil, nil)
+ healthResp := health.Health()
Review comment:
异常时需要返回5xx错误
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780088426
##########
File path: client/set.go
##########
@@ -1,16 +1,25 @@
package client
import (
+ "errors"
"fmt"
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
"google.golang.org/grpc"
+ "google.golang.org/grpc/resolver"
+ "google.golang.org/grpc/resolver/manual"
+)
+
+var (
+ ErrAddrEmpty = errors.New("addr is empty")
+ ErrConnNil = errors.New("conn is nil")
)
// SetConfig is client configs
type SetConfig struct {
- Addr string
+ Addr []string
Review comment:
复数，还是全拼吧
##########
File path: client/set.go
##########
@@ -1,16 +1,25 @@
package client
import (
+ "errors"
"fmt"
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
"google.golang.org/grpc"
+ "google.golang.org/grpc/resolver"
+ "google.golang.org/grpc/resolver/manual"
+)
+
+var (
+ ErrAddrEmpty = errors.New("addr is empty")
+ ErrConnNil = errors.New("conn is nil")
)
// SetConfig is client configs
type SetConfig struct {
- Addr string
+ Addr []string
+ Scheme string
Review comment:
如何理解schema字段的设计，枚举值有哪些
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780644892
##########
File path: syncer/resource/admin/admin.go
##########
@@ -20,20 +20,37 @@ package admin
import (
"net/http"
+ "github.com/apache/servicecomb-service-center/pkg/log"
"github.com/apache/servicecomb-service-center/pkg/rest"
+ "github.com/apache/servicecomb-service-center/server/handler/exception"
+ "github.com/apache/servicecomb-service-center/syncer/service/admin"
+ "github.com/go-chassis/cari/discovery"
)
+const (
+ APIHealth = "/v1/syncer/health"
+)
+
+func init() {
+ exception.RegisterWhitelist(http.MethodGet, APIHealth)
Review comment:
应该是rbac.Add2Whitelist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] xiaoluoluo commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
xiaoluoluo commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780154763
##########
File path: client/set.go
##########
@@ -1,16 +1,25 @@
package client
import (
+ "errors"
"fmt"
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
"google.golang.org/grpc"
+ "google.golang.org/grpc/resolver"
+ "google.golang.org/grpc/resolver/manual"
+)
+
+var (
+ ErrAddrEmpty = errors.New("addr is empty")
+ ErrConnNil = errors.New("conn is nil")
)
// SetConfig is client configs
type SetConfig struct {
- Addr string
+ Addr []string
+ Scheme string
Review comment:
调用请求的时候 会 注册上 解析器 schema 是解析器的名字，这里可以根据自己的业务自己定义
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
+ for _, a := range c.Addr {
+ addr = append(addr, resolver.Address{Addr: a})
+ }
+ r := manual.NewBuilderWithScheme(c.Scheme)
+ r.InitialState(resolver.State{Addresses: addr})
+ conn, err = grpc.Dial(r.Scheme()+":///", grpc.WithInsecure(), grpc.WithResolvers(r))
Review comment:
这里在请求的时候 grpc.WithResolvers(r)) 带上解析器
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#issuecomment-1006501487
> #1206 add health api
PR描述给出有价值的信息，参考下我的提交
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779973126
##########
File path: client/set.go
##########
@@ -20,11 +24,33 @@ type Set struct {
// NewSetForConfig dial grpc connection and create all grpc clients
func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+ if len(c.Addr) <= 0 {
+ return nil, errors.New("addr is empty")
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
+ for _, a := range c.Addr {
+ addr = append(addr, resolver.Address{Addr: a})
+ }
+ r := manual.NewBuilderWithScheme(c.Scheme)
+ r.InitialState(resolver.State{Addresses: addr})
+ conn, err = grpc.Dial(r.Scheme()+":///", grpc.WithInsecure(), grpc.WithResolvers(r))
+ }
+
if err != nil {
log.Error(fmt.Sprintf("can not connect: %s", err), nil)
return nil, err
}
+ if conn == nil {
+ return nil, errors.New("conn is nil")
Review comment:
这个也是一样，主要是为了方便共用
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780089309
##########
File path: etc/conf/app.yaml
##########
@@ -144,6 +144,10 @@ registry:
# the allowable minimum value of instance heartbeat interval
# if interval < minInterval, instance TTL still set with minInterval
minInterval: 5s
+ datacenter:
Review comment:
确保这些配置项和sync下的一模一样
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] tianxiaoliang commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780086179
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
Review comment:
不必维护分支，都是resolver，之所以可以代码合一是因为用例就一个：集群
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
Review comment:
不必维护分支，都是resolver，之所以可以代码合一是因为用例就一个：拨集群
##########
File path: client/set.go
##########
@@ -19,13 +28,35 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
+ if len(c.Addr) <= 0 {
+ return nil, nil, ErrAddrEmpty
+ }
+
+ var conn *grpc.ClientConn
+ var err error
+
+ if len(c.Addr) <= 1 {
+ conn, err = grpc.Dial(c.Addr[0], grpc.WithInsecure())
+ } else {
+ addr := make([]resolver.Address, 0, len(c.Addr))
+ for _, a := range c.Addr {
+ addr = append(addr, resolver.Address{Addr: a})
+ }
+ r := manual.NewBuilderWithScheme(c.Scheme)
+ r.InitialState(resolver.State{Addresses: addr})
+ conn, err = grpc.Dial(r.Scheme()+":///", grpc.WithInsecure(), grpc.WithResolvers(r))
Review comment:
为什么有3个/ 给下标准的依据
##########
File path: client/set.go
##########
@@ -1,16 +1,25 @@
package client
import (
+ "errors"
"fmt"
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
"google.golang.org/grpc"
+ "google.golang.org/grpc/resolver"
+ "google.golang.org/grpc/resolver/manual"
+)
+
+var (
+ ErrAddrEmpty = errors.New("addr is empty")
+ ErrConnNil = errors.New("conn is nil")
)
// SetConfig is client configs
type SetConfig struct {
- Addr string
+ Addr []string
Review comment:
复数，还是全拼吧
##########
File path: client/set.go
##########
@@ -1,16 +1,25 @@
package client
import (
+ "errors"
"fmt"
v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
"github.com/apache/servicecomb-service-center/pkg/log"
"google.golang.org/grpc"
+ "google.golang.org/grpc/resolver"
+ "google.golang.org/grpc/resolver/manual"
+)
+
+var (
+ ErrAddrEmpty = errors.New("addr is empty")
+ ErrConnNil = errors.New("conn is nil")
)
// SetConfig is client configs
type SetConfig struct {
- Addr string
+ Addr []string
+ Scheme string
Review comment:
如何理解schema字段的设计，枚举值有哪些
##########
File path: etc/conf/app.yaml
##########
@@ -144,6 +144,10 @@ registry:
# the allowable minimum value of instance heartbeat interval
# if interval < minInterval, instance TTL still set with minInterval
minInterval: 5s
+ datacenter:
Review comment:
这数据是否和sync路径下的配置冗余了
##########
File path: etc/conf/app.yaml
##########
@@ -144,6 +144,10 @@ registry:
# the allowable minimum value of instance heartbeat interval
# if interval < minInterval, instance TTL still set with minInterval
minInterval: 5s
+ datacenter:
Review comment:
确保这些配置项和sync下的一模一样
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780621502
##########
File path: client/set.go
##########
@@ -19,13 +32,29 @@ type Set struct {
}
// NewSetForConfig dial grpc connection and create all grpc clients
-func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+func NewSetForConfig(c SetConfig) (*grpc.ClientConn, *Set, error) {
Review comment:
三个返回值是不是不太好，一般是2个，华哥怎么看
##########
File path: server/rpc/sync/server.go
##########
@@ -16,3 +22,12 @@ func (s *Server) Sync(ctx context.Context, events *v1sync.EventList) (*v1sync.Re
log.Info(fmt.Sprintf("Received: %v", events.Events[0].Action))
return &v1sync.Results{}, nil
}
+
+func (s *Server) Health(ctx context.Context, request *v1sync.HealthRequest) (*v1sync.HealthReply, error) {
+ log.Info(fmt.Sprintf("Health Received"))
Review comment:
这边如果不打印变量的话可以直接"Health Received"，不需要fmt.Sprintf
##########
File path: syncer/resource/admin/admin.go
##########
@@ -18,22 +18,39 @@
package admin
import (
+ "github.com/apache/servicecomb-service-center/server/handler/exception"
Review comment:
这个是我个人放的顺序，第一个是官方包 第二个顺序是第三方依赖 第三个顺序是项目
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779972829
##########
File path: client/set.go
##########
@@ -20,11 +24,33 @@ type Set struct {
// NewSetForConfig dial grpc connection and create all grpc clients
func NewSetForConfig(c SetConfig) (*Set, error) {
- conn, err := grpc.Dial(c.Addr, grpc.WithInsecure())
+ if len(c.Addr) <= 0 {
+ return nil, errors.New("addr is empty")
Review comment:
这个errors.New("addr is empty")，可以抽成一个 var ErrAddrEmpty = errors.New("addr is empty")
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] robotLJW commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
robotLJW commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r780622103
##########
File path: syncer/resource/admin/admin.go
##########
@@ -18,22 +18,39 @@
package admin
import (
+ "github.com/apache/servicecomb-service-center/server/handler/exception"
Review comment:
这个是我个人放的顺序，第一个是官方包 第二个顺序是第三方依赖 第三个顺序是项目
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #1205: [feat]add health api in sync package
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #1205:
URL: https://github.com/apache/servicecomb-service-center/pull/1205#discussion_r779987452
##########
File path: syncer/health/health.go
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package health
+
+import (
+ "context"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+)
+
+type Resp struct {
+ DataCenter *DataCenter `json:"datacenter"`
+ Peers []*Peer `json:"peer"`
Review comment:
叫peers
##########
File path: syncer/service/admin/health.go
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package admin
+
+import (
+ "context"
+ "errors"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/pkg/rpc"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+ serviceName = "service-center"
Review comment:
应该叫“syncer”
##########
File path: server/rpc/sync/server.go
##########
@@ -16,3 +23,12 @@ func (s *Server) Sync(ctx context.Context, events *v1sync.EventList) (*v1sync.Re
log.Info(fmt.Sprintf("Received: %v", events.Events[0].Action))
return &v1sync.Results{}, nil
}
+
+func (s *Server) Health(ctx context.Context, request *v1sync.HealthRequest) (*v1sync.HealthReply, error) {
+ log.Info("Health Received")
Review comment:
可以不打日志
##########
File path: syncer/service/admin/health.go
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package admin
+
+import (
+ "context"
+ "errors"
+
+ v1sync "github.com/apache/servicecomb-service-center/api/sync/v1"
+ "github.com/apache/servicecomb-service-center/client"
+ "github.com/apache/servicecomb-service-center/pkg/rpc"
+ "github.com/apache/servicecomb-service-center/server/rpc/sync"
+ "github.com/apache/servicecomb-service-center/syncer/config"
+)
+
+const (
+ scheme = "health_rpc"
+ serviceName = "service-center"
+)
+
+var (
+ ErrConfigIsEmpty = errors.New("sync config is empty")
+)
+
+type Resp struct {
+ Peers []*Peer `json:"peers"`
+}
+
+type Peer struct {
+ Name string `json:"name"`
+ Kind string `json:"kind"`
+ Mode []string `json:"mode"`
+ Endpoints []string `json:"endpoints"`
+ Status string `json:"status"`
+}
+
+func Health() (*Resp, error) {
+
+ config := config.GetConfig()
+ if config.Sync == nil || len(config.Sync.Peers) <= 0 {
+ return nil, ErrConfigIsEmpty
+ }
+
+ resp := &Resp{Peers: make([]*Peer, 0, len(config.Sync.Peers))}
+
+ for _, c := range config.Sync.Peers {
+ if len(c.Endpoints) <= 0 {
+ continue
+ }
+ p := &Peer{
+ Name: c.Name,
+ Kind: c.Kind,
+ Mode: c.Mode,
+ Endpoints: c.Endpoints,
+ }
+ p.Status = getPeerStatus(c.Endpoints)
+ resp.Peers = append(resp.Peers, p)
+ }
+
+ if len(resp.Peers) <= 0 {
+ return nil, ErrConfigIsEmpty
+ }
+
+ return resp, nil
+}
+
+func getPeerStatus(endpoints []string) string {
+ conn, err := rpc.GetRoundRobinLbConn(&rpc.Config{Addrs: endpoints, Scheme: scheme, ServiceName: serviceName})
+ if err != nil || conn == nil {
+ return sync.HealthStatusAbnormal
+ }
+ defer conn.Close()
+
+ set := client.NewSetForConfig(conn)
Review comment:
改名：client.NewSet(conn)
##########
File path: syncer/resource/admin/admin.go
##########
@@ -20,20 +20,37 @@ package admin
import (
"net/http"
+ "github.com/apache/servicecomb-service-center/pkg/log"
"github.com/apache/servicecomb-service-center/pkg/rest"
+ "github.com/apache/servicecomb-service-center/server/handler/exception"
+ "github.com/apache/servicecomb-service-center/syncer/service/admin"
+ "github.com/go-chassis/cari/discovery"
)
+const (
+ APIHealth = "/v1/syncer/health"
+)
+
+func init() {
+ exception.RegisterWhitelist(http.MethodGet, APIHealth)
Review comment:
应该是rbac.Add2Whitelist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org