Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions internal/xds/clients/xdsclient/clientimpl_watchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package xdsclient

import (
"context"
"fmt"

"google.golang.org/grpc/internal/xds/clients/xdsclient/internal/xdsresource"
Expand Down Expand Up @@ -62,15 +63,19 @@ func (c *XDSClient) WatchResource(typeURL, resourceName string, watcher Resource
rType, ok := c.config.ResourceTypes[typeURL]
if !ok {
logger.Warningf("ResourceType implementation for resource type url %v is not found", rType.TypeURL)
watcher.ResourceError(fmt.Errorf("ResourceType implementation for resource type url %v is not found", rType.TypeURL), func() {})
c.serializer.TrySchedule(func(context.Context) {
watcher.ResourceError(fmt.Errorf("ResourceType implementation for resource type url %v is not found", rType.TypeURL), func() {})
})
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you fixing a bug in the existing code here? If so, did you get a chance to check the PR where this was added to see if there was in fact a reason for calling the watch callback outside of the serializer here?

Also, while you are here, it might be worth updating the docstring of this method and the ResourceWatcher interface (there is no mention of the serialization guarantees that are provided by the xDS client when calling methods on this interface). It says the watch will fail if the resource type implementation does not exist. But what does failing a watch mean? Also, it does not say anything about the case where the authority in the resource name is not found in the configuration (also, it should not say bootstrpa, as this is the generic xDS client, and bootstrap is a gRPC xDS client concept). You could even do it as a separate PR (both the bug fix and the fixes to the docstring).

return func() {}
}

n := xdsresource.ParseName(resourceName)
a := c.getAuthorityForResource(n)
if a == nil {
logger.Warningf("Watch registered for name %q of type %q, authority %q is not found", rType.TypeName, resourceName, n.Authority)
watcher.ResourceError(fmt.Errorf("authority %q not found in bootstrap config for resource %q", n.Authority, resourceName), func() {})
c.serializer.TrySchedule(func(context.Context) {
watcher.ResourceError(fmt.Errorf("authority %q not found in bootstrap config for resource %q", n.Authority, resourceName), func() {})
})
return func() {}
}
// The watchResource method on the authority is invoked with n.String()
Expand Down
20 changes: 0 additions & 20 deletions internal/xds/resolver/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,26 +191,6 @@ func verifyNoUpdateFromResolver(ctx context.Context, t *testing.T, stateCh chan
}
}

// waitForErrorFromResolver waits for the resolver to push an error and verifies
// that it matches the expected error and contains the expected node ID.
func waitForErrorFromResolver(ctx context.Context, errCh chan error, wantErr, wantNodeID string) error {
select {
case <-ctx.Done():
return fmt.Errorf("timeout when waiting for error to be propagated to the ClientConn")
case gotErr := <-errCh:
if gotErr == nil {
return fmt.Errorf("got nil error from resolver, want %q", wantErr)
}
if !strings.Contains(gotErr.Error(), wantErr) {
return fmt.Errorf("got error from resolver %q, want %q", gotErr, wantErr)
}
if !strings.Contains(gotErr.Error(), wantNodeID) {
return fmt.Errorf("got error from resolver %q, want nodeID %q", gotErr, wantNodeID)
}
}
return nil
}

func verifyResolverError(gotErr error, wantCode codes.Code, wantErr, wantNodeID string) error {
if gotErr == nil {
return fmt.Errorf("got nil error from resolver, want error with code %v", wantCode)
Expand Down
92 changes: 0 additions & 92 deletions internal/xds/resolver/watch_service.go

This file was deleted.

Loading