CodiumAI-Agent commented on PR #746: URL: https://github.com/apache/dubbo-go-pixiu/pull/746#issuecomment-3260837868
## PR Reviewer Guide ๐ Here are some key observations to aid the review process: <table> <tr><td>โฑ๏ธ <strong>Estimated effort to review</strong>: 4 ๐ต๐ต๐ต๐ตโช</td></tr> <tr><td>๐งช <strong>PR contains tests</strong></td></tr> <tr><td>๐ <strong>Security concerns</strong><br><br> <strong>Sensitive information handling:</strong><br> The Nacos listener parses 'llm-meta.api_keys' from instance metadata and stores them in Endpoint.LLMMeta. Ensure logs never print raw keys (current logs include endpoint.Name and Address, not keys). Confirm that the API keys are not exposed via diagnostics or metrics, and consider masking if any logging of LLMMeta occurs elsewhere.</td></tr> <tr><td>โก <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/apache/dubbo-go-pixiu/pull/746/files#diff-7c8965a7c48e0fa60edbdd154643d6c85d9ff72ea43d536741a82f0bdd0a9a63R167-R213'><strong>Possible Issue</strong></a> The serviceCallback builds endpoint keys as serviceName + '@' + endpoint.ID, but OnRemoveEndpoint uses only endpoint.ID for deletion. Ensure cluster manager DeleteEndpoint expects ID only; otherwise, removals might not target the correct instance when IDs are not globally unique across services. </summary> ```go serviceName := services[0].ServiceName logger.Debugf("Received callback for service: %s with %d instances", serviceName, len(services)) oldCache, _ := l.instanceCache.LoadOrStore(serviceName, &sync.Map{}) oldInstanceMap := oldCache.(*sync.Map) newInstanceMap := &sync.Map{} newEndpoints := make(map[string]*model.Endpoint) // Process the new list from Nacos. for i := range services { // Also check for health if !services[i].Enable || !services[i].Healthy { continue } instance := generateInstance(services[i]) endpoint := generateEndpoint(instance) if endpoint == nil { continue } key := serviceName + constant.At + endpoint.ID newInstanceMap.Store(key, instance) newEndpoints[key] = endpoint } // Check for added or updated instances. for key, endpoint := range newEndpoints { if oldRaw, ok := oldInstanceMap.Load(key); ok { oldInstance := oldRaw.(nacosModel.Instance) newInstance, _ := newInstanceMap.Load(key) if !reflect.DeepEqual(oldInstance, newInstance) { l.handle(endpoint, remoting.EventTypeUpdate) } } else { l.handle(endpoint, remoting.EventTypeAdd) } } // Check for removed instances. oldInstanceMap.Range(func(key, value any) bool { instanceKey := key.(string) if _, ok := newEndpoints[instanceKey]; !ok { instance := value.(nacosModel.Instance) endpoint := generateEndpoint(instance) l.handle(endpoint, remoting.EventTypeDel) } ``` </details> <details><summary><a href='https://github.com/apache/dubbo-go-pixiu/pull/746/files#diff-9c9f5ad8f7a29664c69f45eb02ae214541b391eae414816ac79d2dbc6a2ae72aR56-R89'><strong>Backward Compatibility</strong></a> newCountBasedRetry now accepts string/float64 for 'times'. Verify existing configs (mapstructure/yaml) still pass integers as before and that zero/negative handling matches previous semantics. Also confirm tests cover float string and boundary values. </summary> ```go func newCountBasedRetry(config map[string]any) (retry.RetryPolicy, error) { timesValue, exists := config[retryTimesKey] if !exists { timesValue = defaultRetryTimes } var times int switch v := timesValue.(type) { case int: times = v case float64: if v != float64(int(v)) { return nil, fmt.Errorf("invalid float value for '%s', must be a whole number, but got %f", retryTimesKey, v) } times = int(v) case string: parsedTimes, err := strconv.Atoi(v) if err != nil { return nil, fmt.Errorf("could not parse string value for '%s': %w", retryTimesKey, err) } times = parsedTimes default: return nil, fmt.Errorf("invalid type for '%s', expected a number or a numeric string, but got %T", retryTimesKey, timesValue) } // must be non-negative if times < 0 { return nil, fmt.Errorf("value for '%s' cannot be negative, but got %d", retryTimesKey, times) } // Total attempts = 1 initial try plus number of retries. return &CountBasedRetry{MaxAttempts: uint(times) + 1}, nil } ``` </details> <details><summary><a href='https://github.com/apache/dubbo-go-pixiu/pull/746/files#diff-c1f6bf093ce56a0a962e4b4fc42c728bc9a7d37fc3d57e02ad615138002b698bR256-R309'><strong>API Change</strong></a> AssembleClusterEndpoints renamed and refactored to assembleClusterEndpoints plus exported helper AssembleLLMProviderDomains. Validate all external/internal callers updated accordingly and no packages rely on the old exported name. </summary> ```go func (s *ClusterStore) AddCluster(c *model.ClusterConfig) { if c.Name == "" { c.Name = fmt.Sprintf("cluster-%d", clusterIndex) atomic.AddInt32(&clusterIndex, 1) } s.assembleClusterEndpoints(c) s.Config = append(s.Config, c) s.clustersMap[c.Name] = cluster.NewCluster(c) c.CreateConsistentHash() } // assembleClusterEndpoints assembles the cluster endpoints // by formatting the ID, name and domains for each endpoint // If endpoint.LLMMeta is not nil, the assimilation of name and domain is based on // the LLM provider denoted in the endpoint LLMMeta. func (s *ClusterStore) assembleClusterEndpoints(c *model.ClusterConfig) { if c == nil { return } for i, endpoint := range c.Endpoints { // If the endpoint ID is not set, set it to the index + 1 if endpoint.ID == "" { endpoint.ID = strconv.Itoa(i + 1) } // If the endpoint has no name, set a default name if endpoint.Name == "" && endpoint.LLMMeta != nil { endpoint.Name = fmt.Sprintf("endpoint-%d#%s", i+1, endpoint.LLMMeta.Provider) } else if endpoint.Name == "" && endpoint.LLMMeta == nil { endpoint.Name = fmt.Sprintf("endpoint-%d", i+1) } c.Endpoints[i] = AssembleLLMProviderDomains(endpoint) } } func AssembleLLMProviderDomains(endpoint *model.Endpoint) *model.Endpoint { // If the endpoint address and domain are not set, set them based on the LLM provider. // If the endpoint address or domain is set, do not modify them. if endpoint.LLMMeta != nil && endpoint.Address.Address == constant.PprofDefaultAddress && endpoint.Address.Domains == nil { domain, err := model.GetLLMProviderDomains(endpoint.LLMMeta.Provider) if err != nil { logger.Errorf("failed to get llm provider domains, err: %v", err) return endpoint } endpoint.Address.Domains = []string{domain.BaseUrl} } return endpoint } ``` </details> </td></tr> </table> -- 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: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
