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 2020/09/27 08:35:17 UTC
[GitHub] [servicecomb-service-center] popozy opened a new pull request #698: [Enhancement] Implement service/ms get service function
popozy opened a new pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698
### Commit message:
1. optimize register function according to conversation in https://github.com/apache/servicecomb-service-center/pull/697#discussion_r494841829
2. implement getservice && getservices function
3. finish unit case
### history pr:
#695
#697
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] little-cui merged pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
little-cui merged pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495677215
##########
File path: server/rest/controller/v4/microservice_controller.go
##########
@@ -120,6 +120,7 @@ func (s *MicroServiceService) Unregister(w http.ResponseWriter, r *http.Request)
func (s *MicroServiceService) GetServices(w http.ResponseWriter, r *http.Request) {
request := &pb.GetServicesRequest{}
resp, _ := core.ServiceAPI.GetServices(r.Context(), request)
Review comment:
all err check missed has beed added in microservice_controller.go
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495671871
##########
File path: server/service/ms/etcd/etcd.go
##########
@@ -57,78 +52,94 @@ func (ds *DataSource) initialize() error {
}
// RegisterService() implement:
-// 1. capsule request to etcd kv format
-// 2. invoke registry interface to store service information to etcd cluster
-// Attention: parameters validation && response check must be checked outside the method
+// 1. request validation
+// 2. capsule request to etcd kv format
+// 3. invoke etcd client to store data
+// 4. check etcd-client response && construct createServiceResponse
func (ds *DataSource) RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (
- *registry.PluginResponse, error) {
+ *pb.CreateServiceResponse, error) {
+ if request == nil || request.Service == nil {
+ log.Errorf(nil, "create micro-service failed: request body is empty")
+ return &pb.CreateServiceResponse{
+ Response: proto.CreateResponse(scerr.ErrInvalidParams, "Request body is empty"),
+ }, nil
+ }
+
+ // start to store service to etcd
remoteIP := util.GetIPFromContext(ctx)
serviceBody := request.Service
serviceFlag := util.StringJoin([]string{
serviceBody.Environment, serviceBody.AppId, serviceBody.ServiceName, serviceBody.Version}, "/")
- serviceUtil.SetServiceDefaultValue(serviceBody)
-
- domainProject := util.ParseDomainProject(ctx)
+ // request validation
+ err := service.Validate(request)
Review comment:
Do you means that, this logic should be moved to api bussiness layer (invoker of datasource module), not storage module?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495676851
##########
File path: server/service/ms/etcd/etcd.go
##########
@@ -57,78 +52,94 @@ func (ds *DataSource) initialize() error {
}
// RegisterService() implement:
-// 1. capsule request to etcd kv format
-// 2. invoke registry interface to store service information to etcd cluster
-// Attention: parameters validation && response check must be checked outside the method
+// 1. request validation
+// 2. capsule request to etcd kv format
+// 3. invoke etcd client to store data
+// 4. check etcd-client response && construct createServiceResponse
func (ds *DataSource) RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (
- *registry.PluginResponse, error) {
+ *pb.CreateServiceResponse, error) {
+ if request == nil || request.Service == nil {
+ log.Errorf(nil, "create micro-service failed: request body is empty")
+ return &pb.CreateServiceResponse{
+ Response: proto.CreateResponse(scerr.ErrInvalidParams, "Request body is empty"),
+ }, nil
+ }
+
+ // start to store service to etcd
remoteIP := util.GetIPFromContext(ctx)
serviceBody := request.Service
serviceFlag := util.StringJoin([]string{
serviceBody.Environment, serviceBody.AppId, serviceBody.ServiceName, serviceBody.Version}, "/")
- serviceUtil.SetServiceDefaultValue(serviceBody)
-
- domainProject := util.ParseDomainProject(ctx)
+ // request validation
+ err := service.Validate(request)
Review comment:
I check it here based the conception that the input from invoker can nerver been trust.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495750417
##########
File path: server/service/ms/etcd/etcd.go
##########
@@ -57,78 +52,94 @@ func (ds *DataSource) initialize() error {
}
// RegisterService() implement:
-// 1. capsule request to etcd kv format
-// 2. invoke registry interface to store service information to etcd cluster
-// Attention: parameters validation && response check must be checked outside the method
+// 1. request validation
+// 2. capsule request to etcd kv format
+// 3. invoke etcd client to store data
+// 4. check etcd-client response && construct createServiceResponse
func (ds *DataSource) RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (
- *registry.PluginResponse, error) {
+ *pb.CreateServiceResponse, error) {
+ if request == nil || request.Service == nil {
+ log.Errorf(nil, "create micro-service failed: request body is empty")
+ return &pb.CreateServiceResponse{
+ Response: proto.CreateResponse(scerr.ErrInvalidParams, "Request body is empty"),
+ }, nil
+ }
+
+ // start to store service to etcd
remoteIP := util.GetIPFromContext(ctx)
serviceBody := request.Service
serviceFlag := util.StringJoin([]string{
serviceBody.Environment, serviceBody.AppId, serviceBody.ServiceName, serviceBody.Version}, "/")
- serviceUtil.SetServiceDefaultValue(serviceBody)
-
- domainProject := util.ParseDomainProject(ctx)
+ // request validation
+ err := service.Validate(request)
Review comment:
validation in datasource has been moved
----------------------------------------------------------------
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.
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 #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
tianxiaoliang commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495547579
##########
File path: server/service/ms/datasource.go
##########
@@ -18,14 +18,14 @@ package ms
import (
"context"
pb "github.com/apache/servicecomb-service-center/pkg/registry"
- "github.com/apache/servicecomb-service-center/server/plugin/registry"
)
type DataSource interface {
- RegisterService(ctx context.Context, service *pb.CreateServiceRequest) (*registry.PluginResponse, error)
- GetService(ctx context.Context, service *pb.GetServiceRequest)
- UpdateService(ctx context.Context, service *pb.UpdateServicePropsRequest)
- UnregisterService(ctx context.Context, service *pb.DeleteServiceRequest)
+ RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (*pb.CreateServiceResponse, error)
+ GetServices(ctx context.Context, request *pb.GetServicesRequest) (*pb.GetServicesResponse, error)
+ GetService(ctx context.Context, request *pb.GetServiceRequest) (*pb.GetServiceResponse, error)
+ UpdateService(ctx context.Context, request *pb.UpdateServicePropsRequest)
Review comment:
UpdateService,UnregisterService should return response
##########
File path: server/rest/controller/v4/microservice_controller.go
##########
@@ -120,6 +120,7 @@ func (s *MicroServiceService) Unregister(w http.ResponseWriter, r *http.Request)
func (s *MicroServiceService) GetServices(w http.ResponseWriter, r *http.Request) {
request := &pb.GetServicesRequest{}
resp, _ := core.ServiceAPI.GetServices(r.Context(), request)
Review comment:
check error
##########
File path: server/service/ms/etcd/etcd.go
##########
@@ -57,78 +52,94 @@ func (ds *DataSource) initialize() error {
}
// RegisterService() implement:
-// 1. capsule request to etcd kv format
-// 2. invoke registry interface to store service information to etcd cluster
-// Attention: parameters validation && response check must be checked outside the method
+// 1. request validation
+// 2. capsule request to etcd kv format
+// 3. invoke etcd client to store data
+// 4. check etcd-client response && construct createServiceResponse
func (ds *DataSource) RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (
- *registry.PluginResponse, error) {
+ *pb.CreateServiceResponse, error) {
+ if request == nil || request.Service == nil {
+ log.Errorf(nil, "create micro-service failed: request body is empty")
+ return &pb.CreateServiceResponse{
+ Response: proto.CreateResponse(scerr.ErrInvalidParams, "Request body is empty"),
+ }, nil
+ }
+
+ // start to store service to etcd
remoteIP := util.GetIPFromContext(ctx)
serviceBody := request.Service
serviceFlag := util.StringJoin([]string{
serviceBody.Environment, serviceBody.AppId, serviceBody.ServiceName, serviceBody.Version}, "/")
- serviceUtil.SetServiceDefaultValue(serviceBody)
-
- domainProject := util.ParseDomainProject(ctx)
+ // request validation
+ err := service.Validate(request)
Review comment:
validation should be common logic for any storage
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495669840
##########
File path: server/service/ms/datasource.go
##########
@@ -18,14 +18,14 @@ package ms
import (
"context"
pb "github.com/apache/servicecomb-service-center/pkg/registry"
- "github.com/apache/servicecomb-service-center/server/plugin/registry"
)
type DataSource interface {
- RegisterService(ctx context.Context, service *pb.CreateServiceRequest) (*registry.PluginResponse, error)
- GetService(ctx context.Context, service *pb.GetServiceRequest)
- UpdateService(ctx context.Context, service *pb.UpdateServicePropsRequest)
- UnregisterService(ctx context.Context, service *pb.DeleteServiceRequest)
+ RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (*pb.CreateServiceResponse, error)
+ GetServices(ctx context.Context, request *pb.GetServicesRequest) (*pb.GetServicesResponse, error)
+ GetService(ctx context.Context, request *pb.GetServiceRequest) (*pb.GetServiceResponse, error)
+ UpdateService(ctx context.Context, request *pb.UpdateServicePropsRequest)
Review comment:
This pr finish getservices/getservice interface with its ut.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495751211
##########
File path: server/rest/controller/v4/microservice_controller.go
##########
@@ -113,13 +113,23 @@ func (s *MicroServiceService) Unregister(w http.ResponseWriter, r *http.Request)
ServiceId: serviceID,
Force: b,
}
- resp, _ := core.ServiceAPI.Delete(r.Context(), request)
+ resp, err := core.ServiceAPI.Delete(r.Context(), request)
+ if err != nil {
+ log.Errorf(err, "delete service[%s] failed", serviceID)
+ controller.WriteError(w, scerr.ErrInternal, "delete service failed")
+ return
+ }
controller.WriteResponse(w, resp.Response, nil)
Review comment:
I'll abstract the interfaces declared in v4.go in the future pr, considering the workload.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495676301
##########
File path: server/service/ms/datasource.go
##########
@@ -18,14 +18,14 @@ package ms
import (
"context"
pb "github.com/apache/servicecomb-service-center/pkg/registry"
- "github.com/apache/servicecomb-service-center/server/plugin/registry"
)
type DataSource interface {
- RegisterService(ctx context.Context, service *pb.CreateServiceRequest) (*registry.PluginResponse, error)
- GetService(ctx context.Context, service *pb.GetServiceRequest)
- UpdateService(ctx context.Context, service *pb.UpdateServicePropsRequest)
- UnregisterService(ctx context.Context, service *pb.DeleteServiceRequest)
+ RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (*pb.CreateServiceResponse, error)
+ GetServices(ctx context.Context, request *pb.GetServicesRequest) (*pb.GetServicesResponse, error)
+ GetService(ctx context.Context, request *pb.GetServiceRequest) (*pb.GetServiceResponse, error)
+ UpdateService(ctx context.Context, request *pb.UpdateServicePropsRequest)
Review comment:
done, has add response
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495669542
##########
File path: server/service/ms/datasource.go
##########
@@ -18,14 +18,14 @@ package ms
import (
"context"
pb "github.com/apache/servicecomb-service-center/pkg/registry"
- "github.com/apache/servicecomb-service-center/server/plugin/registry"
)
type DataSource interface {
- RegisterService(ctx context.Context, service *pb.CreateServiceRequest) (*registry.PluginResponse, error)
- GetService(ctx context.Context, service *pb.GetServiceRequest)
- UpdateService(ctx context.Context, service *pb.UpdateServicePropsRequest)
- UnregisterService(ctx context.Context, service *pb.DeleteServiceRequest)
+ RegisterService(ctx context.Context, request *pb.CreateServiceRequest) (*pb.CreateServiceResponse, error)
+ GetServices(ctx context.Context, request *pb.GetServicesRequest) (*pb.GetServicesResponse, error)
+ GetService(ctx context.Context, request *pb.GetServiceRequest) (*pb.GetServiceResponse, error)
+ UpdateService(ctx context.Context, request *pb.UpdateServicePropsRequest)
Review comment:
In todo list, I'm now implementing them
----------------------------------------------------------------
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.
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 #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
little-cui commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495725525
##########
File path: server/rest/controller/v4/microservice_controller.go
##########
@@ -113,13 +113,23 @@ func (s *MicroServiceService) Unregister(w http.ResponseWriter, r *http.Request)
ServiceId: serviceID,
Force: b,
}
- resp, _ := core.ServiceAPI.Delete(r.Context(), request)
+ resp, err := core.ServiceAPI.Delete(r.Context(), request)
+ if err != nil {
+ log.Errorf(err, "delete service[%s] failed", serviceID)
+ controller.WriteError(w, scerr.ErrInternal, "delete service failed")
+ return
+ }
controller.WriteResponse(w, resp.Response, nil)
Review comment:
plz new a uitl function to wrap line 116-122
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-service-center] popozy commented on a change in pull request #698: [Enhancement] Implement service/ms get service function
Posted by GitBox <gi...@apache.org>.
popozy commented on a change in pull request #698:
URL: https://github.com/apache/servicecomb-service-center/pull/698#discussion_r495732782
##########
File path: server/rest/controller/v4/microservice_controller.go
##########
@@ -113,13 +113,23 @@ func (s *MicroServiceService) Unregister(w http.ResponseWriter, r *http.Request)
ServiceId: serviceID,
Force: b,
}
- resp, _ := core.ServiceAPI.Delete(r.Context(), request)
+ resp, err := core.ServiceAPI.Delete(r.Context(), request)
+ if err != nil {
+ log.Errorf(err, "delete service[%s] failed", serviceID)
+ controller.WriteError(w, scerr.ErrInternal, "delete service failed")
+ return
+ }
controller.WriteResponse(w, resp.Response, nil)
Review comment:
just for this deleteService method? or for all methods which implement interface declared in v4.go?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org