fixed,Thanks

There may be concurrency issues here. If more than one worker establish connections at the same time, a connection may be closed multiple times.

fixed

why add this lock? it seems that all operations are protected by this write lock c.lock.

agree, Continuing to use lock in estimatorCache is sufficient.
Would it be better to modify it like this?

// EstablishConnection establishes a new gRPC connection with the specified cluster scheduler estimator.
func EstablishConnection(kubeClient kubernetes.Interface, name string, estimatorCache *SchedulerEstimatorCache, estimatorServicePrefix string, grpcConfig *grpcconnection.ClientConfig) error {
	serverAddr, err := resolveCluster(kubeClient, util.NamespaceKarmadaSystem,
		names.GenerateEstimatorServiceName(estimatorServicePrefix, name), int32(grpcConfig.TargetPort))
	if err != nil {
		return err
	}

	klog.Infof("Start dialing estimator server(%s) of cluster(%s).", serverAddr, name)
	cc, err := grpcConfig.DialWithTimeOut(serverAddr, 5*time.Second)
	if err != nil {
		klog.Errorf("Failed to dial cluster(%s): %v.", name, err)
		return err
	}
	c := estimatorservice.NewEstimatorClient(cc)
	estimatorCache.AddOrUpdateCluster(name, cc, c)
	klog.Infof("Connection with estimator server(%s) of cluster(%s) has been established.", serverAddr, name)
	return nil
}

// AddOrUpdateCluster adds or updates a grpc connection and associated client into the cache.
func (c *SchedulerEstimatorCache) AddOrUpdateCluster(name string, connection *grpc.ClientConn, client estimatorservice.EstimatorClient) {
	c.lock.Lock()
	defer c.lock.Unlock()

	if _, exist := c.estimator[name]; exist {
		_ = c.estimator[name].connection.Close()
	}
	c.estimator[name] = &clientWrapper{
		connection: connection,
		client:     client,
	}
}

make sense，Thanks

is it possible that multiple workers can acquire this lock estimatorCache.lock at the same time?

BTW, I think this close function es.connection.Close() is thread safe.

I think this close function es.connection.Close() is thread safe.

It is indeed thread-safe, but it may cause some unused connections to not be closed properly.

is it possible that multiple workers can acquire this lock estimatorCache.lock at the same time?

EstablishConnection is uesd as a part of ReconcileFunc of scheduler/descheduler.schedulerEstimatorWorker, and has the ability to run concurrently.

There is no need to remove this method. It is an exported function and may be used elsewhere.

done,Thanks~

if the cluster state changes frequently, I am worried that this modification will cause the connection to be updated too frequently. that would be bad.

agree, updating too often is also a problem, unless it has to be updated.
@zhy76 Can you remind me what the problem is if the scheduler estimator cache is not updated when the cluster changes? Does it have to be updated every time? Or is it possible to update only part of it? For example, does the connection need to be updated?

I've reconsidered, and it seems there is no impact. It looks like the author watches for updates only to ensure that all clusters are added to the cache. Only uses the cluster name to find the corresponding estimator server's service and has no other usage. This PR may no longer be necessary. Thank you all for your help. Ask @Garrybest for check and final decision.

Exactly, the cluster name is used to connect the corresponding estimator service in host cluster. I think there is no such circumstance to update this service, right?

Meanwhile, a cluster status could change frequently, so too much unnecessary connection establishment could be expensive.

Thanks for your reply~

suggest:

if oldConn, exist := c.estimator[name]; exist {
    oldConn.Close()
}