[GitHub] [servicecomb-service-center] little-cui commented on a change in pull request #953: SCB-2094 Fast register with mongo

[GitBox <gi...@apache.org>]

little-cui commented on a change in pull request #953:
URL: https://github.com/apache/servicecomb-service-center/pull/953#discussion_r615510635



##########
File path: datasource/mongo/ms.go
##########
@@ -1852,7 +1916,7 @@ func (ds *DataSource) UnregisterInstance(ctx context.Context, request *discovery
 func (ds *DataSource) Heartbeat(ctx context.Context, request *discovery.HeartbeatRequest) (*discovery.HeartbeatResponse, error) {
 	remoteIP := util.GetIPFromContext(ctx)
 	instanceFlag := util.StringJoin([]string{request.ServiceId, request.InstanceId}, "/")
-	err := KeepAliveLease(ctx, request)
+	err := KeepAliveLease(request)

Review comment:
       不应该去掉ctx，否则上层请求无法主动终止流程

##########
File path: datasource/mongo/ms.go
##########
@@ -1964,7 +2028,7 @@ func registryInstance(ctx context.Context, request *discovery.RegisterInstanceRe
 		Instance:    instance,
 	}
 
-	insertRes, err := client.GetMongoClient().Insert(ctx, model.CollectionInstance, data)
+	insertRes, err := client.GetMongoClient().Insert(context.TODO(), model.CollectionInstance, data)

Review comment:
       同上，这种会导致阻塞gorouting

##########
File path: datasource/mongo/ms.go
##########
@@ -1404,13 +1404,80 @@ func getServiceDetailUtil(ctx context.Context, mgs *model.Service, countOnly boo
 }
 
 // Instance management
-func (ds *DataSource) RegisterInstance(ctx context.Context, request *discovery.RegisterInstanceRequest) (*discovery.RegisterInstanceResponse, error) {
+func (ds *DataSource) RegisterInstance(ctx context.Context,
+	request *discovery.RegisterInstanceRequest) (*discovery.RegisterInstanceResponse, error) {
+
+	isCustomID := true
+
+	if len(request.Instance.InstanceId) == 0 {
+		isCustomID = false
+		request.Instance.InstanceId = uuid.Generator().GetInstanceID(ctx)
+	}
+
+	if isFastRegisterEnabled {
+		// fast register, just add instance to channel and batch register them later
+		event := &InstanceRegisterEvent{ctx, request, isCustomID, 0}
+		GetFastRegisterInstanceService().AddEvent(event)

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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org