xdsclient: include xds node ID in errors from the WatchResource API #8093
Conversation
xds/internal/xdsclient/authority.go
Outdated
| if a.activeXDSChannel != nil { | ||
| return a.activeXDSChannel | ||
| return a.activeXDSChannel, nil | ||
| } | ||
|
|
||
| sc := a.xdsChannelConfigs[0].serverConfig | ||
| xc, cleanup, err := a.getChannelForADS(sc, a) | ||
| if err != nil { | ||
| a.logger.Warningf("Failed to create xDS channel: %v", err) |
There was a problem hiding this comment.
Do we still want a warning here if we're also returning an error?
There was a problem hiding this comment.
Fair point. Removed.
| @@ -45,7 +45,9 @@ func (c *clientImpl) WatchResource(rType xdsresource.Type, resourceName string, | |||
|
|
|||
| if err := c.resourceTypes.maybeRegister(rType); err != nil { | |||
| logger.Warningf("Watch registered for name %q of type %q which is already registered", rType.TypeName(), resourceName) | |||
| c.serializer.TrySchedule(func(context.Context) { watcher.OnError(err, func() {}) }) | |||
| c.serializer.TrySchedule(func(context.Context) { | |||
| watcher.OnError(fmt.Errorf("[xDS node id: %v]: %v", c.nodeID, err), func() {}) | |||
There was a problem hiding this comment.
Wondering if we should do something so that watcher.OnError always prefixes the node id automatically?
watcher := c.wrapWatcher(watcher)or something?
There was a problem hiding this comment.
Thanks for the idea. This definitely reduces the amount of change in this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8093 +/- ##
==========================================
+ Coverage 82.31% 82.35% +0.03%
==========================================
Files 387 387
Lines 38962 38967 +5
==========================================
+ Hits 32073 32091 +18
+ Misses 5578 5568 -10
+ Partials 1311 1308 -3
|
| func (w *wrappingWatcher) OnUpdate(update xdsresource.ResourceData, done xdsresource.OnDoneFunc) { | ||
| w.watcher.OnUpdate(update, done) | ||
| } |
There was a problem hiding this comment.
Optional: embed and delete the pass-through methods? (i.e. make the PR even smaller 😄 )
There was a problem hiding this comment.
Sigh. I promise, that was the first thing I thought about and somehow my brain told me that I can't do it. I blame it on the ongoing Rust learning :P
Addresses first bullet point in #7931
RELEASE NOTES: none