xdsclient: include xds node ID in errors from the WatchResource API #8093
Conversation
xds/internal/xdsclient/authority.go Outdated
| 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.
| 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
2 participants
Addresses first bullet point in #7931
RELEASE NOTES: none