Copilot commented on code in PR #849: URL: https://github.com/apache/dubbo-go-pixiu/pull/849#discussion_r2638204330
########## pkg/filter/network/grpcproxy/filter/proxy/dynamic_codec.go: ########## @@ -0,0 +1,232 @@ +/* + * 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 proxy + +import ( + "fmt" +) + +import ( + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/dynamicpb" +) + +// DynamicCodec is a gRPC codec that uses protobuf reflection to encode/decode messages. +// This enables the gateway to inspect and manipulate message content. +type DynamicCodec struct { + methodDesc protoreflect.MethodDescriptor +} + +// NewDynamicCodec creates a new DynamicCodec for the given method descriptor +func NewDynamicCodec(methodDesc protoreflect.MethodDescriptor) *DynamicCodec { + return &DynamicCodec{ + methodDesc: methodDesc, + } +} + +// Marshal encodes a message to bytes +func (c *DynamicCodec) Marshal(v any) ([]byte, error) { + switch msg := v.(type) { + case *dynamicpb.Message: + return proto.Marshal(msg) + case proto.Message: + return proto.Marshal(msg) + case []byte: + // Already bytes, pass through + return msg, nil + case *DynamicMessage: + return proto.Marshal(msg.Message) + default: + return nil, fmt.Errorf("dynamic codec: cannot marshal type %T", v) + } +} + +// Unmarshal decodes bytes into a message +func (c *DynamicCodec) Unmarshal(data []byte, v any) error { + switch msg := v.(type) { + case **DynamicMessage: + // Create dynamic message for input type + inputType := c.methodDesc.Input() + dynMsg := dynamicpb.NewMessage(inputType) + if err := proto.Unmarshal(data, dynMsg); err != nil { + return fmt.Errorf("dynamic codec: unmarshal error: %w", err) + } + *msg = &DynamicMessage{ + Message: dynMsg, + Descriptor: inputType, + } + return nil + case *[]byte: + // Passthrough mode + *msg = data + return nil + case proto.Message: + return proto.Unmarshal(data, msg) + default: + return fmt.Errorf("dynamic codec: cannot unmarshal into type %T", v) + } +} + +// Name returns the codec name +func (c *DynamicCodec) Name() string { + return "dynamic_proto" +} + +// DynamicMessage wraps a dynamicpb.Message with its descriptor +type DynamicMessage struct { + Message *dynamicpb.Message + Descriptor protoreflect.MessageDescriptor +} + +// GetField retrieves a field value by name +func (dm *DynamicMessage) GetField(name string) (protoreflect.Value, bool) { + fd := dm.Descriptor.Fields().ByName(protoreflect.Name(name)) + if fd == nil { + return protoreflect.Value{}, false + } + return dm.Message.Get(fd), true +} + +// GetFieldString retrieves a string field value by name +func (dm *DynamicMessage) GetFieldString(name string) (string, bool) { + val, ok := dm.GetField(name) + if !ok { + return "", false + } + return val.String(), true +} + +// GetFieldInt retrieves an int64 field value by name +func (dm *DynamicMessage) GetFieldInt(name string) (int64, bool) { + val, ok := dm.GetField(name) + if !ok { + return 0, false + } + return val.Int(), true +} + +// SetField sets a field value by name +func (dm *DynamicMessage) SetField(name string, value protoreflect.Value) bool { + fd := dm.Descriptor.Fields().ByName(protoreflect.Name(name)) + if fd == nil { + return false + } + dm.Message.Set(fd, value) + return true +} + +// ToBytes serializes the message to bytes +func (dm *DynamicMessage) ToBytes() ([]byte, error) { + return proto.Marshal(dm.Message) +} + +// ToJSON converts the message to JSON (for logging/debugging) +func (dm *DynamicMessage) ToJSON() ([]byte, error) { + // Use protojson for proper JSON encoding + return []byte(dm.Message.String()), nil +} Review Comment: The ToJSON method currently uses Message.String() which returns the proto text format, not actual JSON. The comment says "Use protojson for proper JSON encoding" but the implementation doesn't use protojson. Consider importing "google.golang.org/protobuf/encoding/protojson" and using protojson.Marshal(dm.Message) for proper JSON encoding, or update the comment to reflect that it returns text format. ########## pkg/filter/network/grpcproxy/filter/proxy/descriptor_cache.go: ########## @@ -0,0 +1,129 @@ +/* + * 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 proxy + +import ( + "sync" + "time" +) + +import ( + "google.golang.org/protobuf/reflect/protoreflect" +) + +// cacheEntry represents a cached method descriptor with expiration +type cacheEntry struct { + descriptor protoreflect.MethodDescriptor + expireAt time.Time +} + +// DescriptorCache provides TTL-based caching for gRPC method descriptors +type DescriptorCache struct { + entries sync.Map + ttl time.Duration + stopCh chan struct{} + closeOnce sync.Once +} + +// NewDescriptorCache creates a new descriptor cache with the specified TTL +func NewDescriptorCache(ttl time.Duration) *DescriptorCache { + cache := &DescriptorCache{ + ttl: ttl, + stopCh: make(chan struct{}), + } + go cache.cleanupLoop() + return cache +} + +// Get retrieves a method descriptor from cache +// Returns nil if not found or expired +func (c *DescriptorCache) Get(key string) protoreflect.MethodDescriptor { + if entry, ok := c.entries.Load(key); ok { + e := entry.(*cacheEntry) + if time.Now().Before(e.expireAt) { + return e.descriptor + } + // Entry expired, delete it + c.entries.Delete(key) + } + return nil +} + +// Set stores a method descriptor in the cache +func (c *DescriptorCache) Set(key string, descriptor protoreflect.MethodDescriptor) { + c.entries.Store(key, &cacheEntry{ + descriptor: descriptor, + expireAt: time.Now().Add(c.ttl), + }) +} + +// Delete removes a specific entry from the cache +func (c *DescriptorCache) Delete(key string) { + c.entries.Delete(key) +} + +// Clear removes all entries from the cache +func (c *DescriptorCache) Clear() { + c.entries.Range(func(key, _ any) bool { + c.entries.Delete(key) + return true + }) +} + +// Size returns the number of entries in the cache (including expired ones) +func (c *DescriptorCache) Size() int { + count := 0 + c.entries.Range(func(_, _ any) bool { + count++ + return true + }) + return count +} Review Comment: The Size() method comment states it returns the count "including expired ones", which could be misleading for monitoring purposes. Users calling GetCacheStats() might assume the count represents valid entries. Consider either filtering out expired entries in Size(), or updating the documentation to clarify that this is the total entry count and may include expired entries that haven't been cleaned up yet. ########## pkg/filter/network/grpcproxy/filter/proxy/protocol.go: ########## @@ -0,0 +1,79 @@ +/* + * 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 proxy + +import ( + "strings" +) + +// ProtocolType represents the type of gRPC-compatible protocol +type ProtocolType string + +const ( + // ProtocolGRPC standard gRPC protocol + ProtocolGRPC ProtocolType = "grpc" + // ProtocolTriple Dubbo Triple protocol (gRPC compatible) + ProtocolTriple ProtocolType = "triple" +) + +// Triple-specific header prefix +const ( + TripleHeaderPrefix = "tri-" +) + +// Triple metadata header keys +var TripleMetadataHeaders = []string{ + "tri-service-version", + "tri-service-group", + "tri-unit-info", + "tri-req-id", + "tri-consumer-appname", + "tri-protocol-version", +} Review Comment: The TripleMetadataHeaders variable is defined but never used in the codebase. ExtractTripleMetadata uses the TripleHeaderPrefix instead. Consider removing this variable if it's not needed, or document its intended use if it's meant to be a public API for users of this package. ########## pkg/filter/network/grpcproxy/filter/proxy/reflection_manager.go: ########## @@ -0,0 +1,402 @@ +/* + * 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 proxy + +import ( + "context" + "fmt" + "io" + "sync" + "time" +) + +import ( + "github.com/pkg/errors" + + "google.golang.org/grpc" + rpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protodesc" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" + "google.golang.org/protobuf/types/descriptorpb" +) + +import ( + "github.com/apache/dubbo-go-pixiu/pkg/logger" +) + +const ( + defaultDescCacheTTL = 5 * time.Minute + reflectionTimeout = 10 * time.Second +) + +// ReflectionManager manages gRPC reflection clients and descriptor caching +// using official google.golang.org/protobuf libraries +type ReflectionManager struct { + cache *DescriptorCache + cacheTTL time.Duration + // fileDescCache caches file descriptors per address + fileDescCache sync.Map // address -> *protoregistry.Files + mu sync.RWMutex +} + +// NewReflectionManager creates a new reflection manager +func NewReflectionManager(cacheTTL time.Duration) *ReflectionManager { + if cacheTTL <= 0 { + cacheTTL = defaultDescCacheTTL + } + return &ReflectionManager{ + cache: NewDescriptorCache(cacheTTL), + cacheTTL: cacheTTL, + } +} + +// GetMethodDescriptor retrieves a method descriptor using gRPC reflection +// Results are cached for improved performance +func (rm *ReflectionManager) GetMethodDescriptor( + ctx context.Context, + conn *grpc.ClientConn, + address string, + serviceName string, + methodName string, +) (protoreflect.MethodDescriptor, error) { + // Build cache key + cacheKey := BuildCacheKey(address, serviceName, methodName) + + // Check cache first + if cached := rm.cache.Get(cacheKey); cached != nil { + logger.Debugf("Reflection cache hit for %s", cacheKey) + return cached, nil + } + + logger.Debugf("Reflection cache miss for %s, performing reflection", cacheKey) + + // Perform reflection with timeout + reflectCtx, cancel := context.WithTimeout(ctx, reflectionTimeout) + defer cancel() + + // Get or create file registry for this address + files, err := rm.getOrCreateFileRegistry(reflectCtx, conn, address, serviceName) + if err != nil { + return nil, err + } + + // Find service descriptor + serviceDesc, err := files.FindDescriptorByName(protoreflect.FullName(serviceName)) + if err != nil { + return nil, errors.Wrapf(err, "failed to find service %s", serviceName) + } + + svcDesc, ok := serviceDesc.(protoreflect.ServiceDescriptor) + if !ok { + return nil, fmt.Errorf("%s is not a service", serviceName) + } + + // Find method descriptor + methodDesc := svcDesc.Methods().ByName(protoreflect.Name(methodName)) + if methodDesc == nil { + return nil, fmt.Errorf("method %s not found in service %s", methodName, serviceName) + } + + // Cache the result + rm.cache.Set(cacheKey, methodDesc) + logger.Debugf("Cached method descriptor for %s", cacheKey) + + return methodDesc, nil +} + +// getOrCreateFileRegistry gets or creates a file registry for the given address +func (rm *ReflectionManager) getOrCreateFileRegistry( + ctx context.Context, + conn *grpc.ClientConn, + address string, + serviceName string, +) (*protoregistry.Files, error) { + // Check if we already have a registry for this address + if cached, ok := rm.fileDescCache.Load(address); ok { + return cached.(*protoregistry.Files), nil + } + + rm.mu.Lock() + defer rm.mu.Unlock() + + // Double check after acquiring lock + if cached, ok := rm.fileDescCache.Load(address); ok { + return cached.(*protoregistry.Files), nil + } + + // Create reflection client + client := rpb.NewServerReflectionClient(conn) + stream, err := client.ServerReflectionInfo(ctx) + if err != nil { + return nil, errors.Wrap(err, "failed to create reflection stream") + } + defer stream.CloseSend() + + // Request file descriptor for the service + fileDescs, err := rm.resolveServiceFileDescriptors(stream, serviceName) + if err != nil { + return nil, err + } Review Comment: The reflection stream is created but may not be properly closed in all error paths. While CloseSend() is deferred, if resolveServiceFileDescriptors or buildFileRegistry return an error, the stream might leak resources. Consider also handling stream closure more explicitly or documenting that CloseSend() is sufficient for cleanup. ########## pkg/filter/network/grpcproxy/filter/proxy/protocol.go: ########## @@ -0,0 +1,79 @@ +/* + * 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 proxy + +import ( + "strings" +) + +// ProtocolType represents the type of gRPC-compatible protocol +type ProtocolType string + +const ( + // ProtocolGRPC standard gRPC protocol + ProtocolGRPC ProtocolType = "grpc" + // ProtocolTriple Dubbo Triple protocol (gRPC compatible) + ProtocolTriple ProtocolType = "triple" +) + +// Triple-specific header prefix +const ( + TripleHeaderPrefix = "tri-" +) + +// Triple metadata header keys +var TripleMetadataHeaders = []string{ + "tri-service-version", + "tri-service-group", + "tri-unit-info", + "tri-req-id", + "tri-consumer-appname", + "tri-protocol-version", +} + +// DetectProtocol detects the protocol type from Content-Type header +func DetectProtocol(contentType string) ProtocolType { + ct := strings.ToLower(contentType) + switch { + case strings.Contains(ct, "application/grpc+proto"), + strings.Contains(ct, "application/triple"), + strings.Contains(ct, "application/grpc+triple"): + return ProtocolTriple + default: + return ProtocolGRPC + } +} Review Comment: The DetectProtocol function is defined but not actually used in the main filter code. The filter currently doesn't call this function to determine protocol type. If protocol detection is intended to be part of this feature, consider integrating it into the filter logic. Otherwise, document that this is a utility function for future use. ########## pkg/filter/network/grpcproxy/filter/proxy/reflection_manager.go: ########## @@ -0,0 +1,402 @@ +/* + * 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 proxy + +import ( + "context" + "fmt" + "io" + "sync" + "time" +) + +import ( + "github.com/pkg/errors" + + "google.golang.org/grpc" + rpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protodesc" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" + "google.golang.org/protobuf/types/descriptorpb" +) + +import ( + "github.com/apache/dubbo-go-pixiu/pkg/logger" +) + +const ( + defaultDescCacheTTL = 5 * time.Minute + reflectionTimeout = 10 * time.Second +) + +// ReflectionManager manages gRPC reflection clients and descriptor caching +// using official google.golang.org/protobuf libraries +type ReflectionManager struct { + cache *DescriptorCache + cacheTTL time.Duration + // fileDescCache caches file descriptors per address + fileDescCache sync.Map // address -> *protoregistry.Files + mu sync.RWMutex +} + +// NewReflectionManager creates a new reflection manager +func NewReflectionManager(cacheTTL time.Duration) *ReflectionManager { + if cacheTTL <= 0 { + cacheTTL = defaultDescCacheTTL + } + return &ReflectionManager{ + cache: NewDescriptorCache(cacheTTL), + cacheTTL: cacheTTL, + } +} + +// GetMethodDescriptor retrieves a method descriptor using gRPC reflection +// Results are cached for improved performance +func (rm *ReflectionManager) GetMethodDescriptor( + ctx context.Context, + conn *grpc.ClientConn, + address string, + serviceName string, + methodName string, +) (protoreflect.MethodDescriptor, error) { + // Build cache key + cacheKey := BuildCacheKey(address, serviceName, methodName) + + // Check cache first + if cached := rm.cache.Get(cacheKey); cached != nil { + logger.Debugf("Reflection cache hit for %s", cacheKey) + return cached, nil + } + + logger.Debugf("Reflection cache miss for %s, performing reflection", cacheKey) + + // Perform reflection with timeout + reflectCtx, cancel := context.WithTimeout(ctx, reflectionTimeout) + defer cancel() + + // Get or create file registry for this address + files, err := rm.getOrCreateFileRegistry(reflectCtx, conn, address, serviceName) + if err != nil { + return nil, err + } + + // Find service descriptor + serviceDesc, err := files.FindDescriptorByName(protoreflect.FullName(serviceName)) + if err != nil { + return nil, errors.Wrapf(err, "failed to find service %s", serviceName) + } + + svcDesc, ok := serviceDesc.(protoreflect.ServiceDescriptor) + if !ok { + return nil, fmt.Errorf("%s is not a service", serviceName) + } + + // Find method descriptor + methodDesc := svcDesc.Methods().ByName(protoreflect.Name(methodName)) + if methodDesc == nil { + return nil, fmt.Errorf("method %s not found in service %s", methodName, serviceName) + } + + // Cache the result + rm.cache.Set(cacheKey, methodDesc) + logger.Debugf("Cached method descriptor for %s", cacheKey) + + return methodDesc, nil +} + +// getOrCreateFileRegistry gets or creates a file registry for the given address +func (rm *ReflectionManager) getOrCreateFileRegistry( + ctx context.Context, + conn *grpc.ClientConn, + address string, + serviceName string, +) (*protoregistry.Files, error) { + // Check if we already have a registry for this address + if cached, ok := rm.fileDescCache.Load(address); ok { + return cached.(*protoregistry.Files), nil + } + + rm.mu.Lock() + defer rm.mu.Unlock() + + // Double check after acquiring lock + if cached, ok := rm.fileDescCache.Load(address); ok { + return cached.(*protoregistry.Files), nil + } + + // Create reflection client + client := rpb.NewServerReflectionClient(conn) + stream, err := client.ServerReflectionInfo(ctx) + if err != nil { + return nil, errors.Wrap(err, "failed to create reflection stream") + } + defer stream.CloseSend() + + // Request file descriptor for the service + fileDescs, err := rm.resolveServiceFileDescriptors(stream, serviceName) + if err != nil { + return nil, err + } + + // Build file registry + files, err := rm.buildFileRegistry(fileDescs) + if err != nil { + return nil, err + } + + // Cache the registry + rm.fileDescCache.Store(address, files) + + return files, nil +} + +// resolveServiceFileDescriptors resolves all file descriptors needed for a service +func (rm *ReflectionManager) resolveServiceFileDescriptors( + stream rpb.ServerReflection_ServerReflectionInfoClient, + serviceName string, +) ([]*descriptorpb.FileDescriptorProto, error) { + // Request file containing the service + req := &rpb.ServerReflectionRequest{ + MessageRequest: &rpb.ServerReflectionRequest_FileContainingSymbol{ + FileContainingSymbol: serviceName, + }, + } + + if err := stream.Send(req); err != nil { + return nil, errors.Wrap(err, "failed to send reflection request") + } + + resp, err := stream.Recv() + if err != nil { + return nil, errors.Wrap(err, "failed to receive reflection response") + } + + fdResp := resp.GetFileDescriptorResponse() + if fdResp == nil { + if errResp := resp.GetErrorResponse(); errResp != nil { + return nil, fmt.Errorf("reflection error: %s", errResp.ErrorMessage) + } + return nil, errors.New("unexpected reflection response") + } + + // Parse file descriptors + fileDescs := make([]*descriptorpb.FileDescriptorProto, 0, len(fdResp.FileDescriptorProto)) + for _, fdBytes := range fdResp.FileDescriptorProto { + fd := &descriptorpb.FileDescriptorProto{} + if err := proto.Unmarshal(fdBytes, fd); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal file descriptor") + } + fileDescs = append(fileDescs, fd) + } + + // Resolve dependencies recursively + resolved := make(map[string]bool) + for _, fd := range fileDescs { + resolved[fd.GetName()] = true + } + + for _, fd := range fileDescs { + deps, err := rm.resolveDependencies(stream, fd.GetDependency(), resolved) + if err != nil { + return nil, err + } + fileDescs = append(fileDescs, deps...) + } + + return fileDescs, nil +} + +// resolveDependencies resolves file descriptor dependencies +func (rm *ReflectionManager) resolveDependencies( + stream rpb.ServerReflection_ServerReflectionInfoClient, + deps []string, + resolved map[string]bool, +) ([]*descriptorpb.FileDescriptorProto, error) { + var result []*descriptorpb.FileDescriptorProto + + for _, dep := range deps { + if resolved[dep] { + continue + } + + req := &rpb.ServerReflectionRequest{ + MessageRequest: &rpb.ServerReflectionRequest_FileByFilename{ + FileByFilename: dep, + }, + } + + if err := stream.Send(req); err != nil { + if err == io.EOF { + break + } + return nil, errors.Wrapf(err, "failed to send request for %s", dep) + } + + resp, err := stream.Recv() + if err != nil { + if err == io.EOF { + break + } Review Comment: In the resolveDependencies function, when an error occurs (lines 246-250, 253-258), the code breaks on io.EOF but returns an error otherwise. However, breaking on io.EOF leaves the function to continue the loop, which may result in incomplete dependency resolution. Consider whether io.EOF should be treated as a non-error condition that allows continuing with partial results, or if it should be treated differently to ensure all dependencies are resolved. ```suggestion return nil, errors.Wrapf(err, "failed to send request for %s", dep) } resp, err := stream.Recv() if err != nil { ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
