xds: Changes to XdsClient Watcher APIs #12446
Conversation
| * Called to deliver a transient error that should not affect the watcher's use of any | ||
| * previously received resource. | ||
| * | ||
| * <p>Note that we expect that the implementer to: |
There was a problem hiding this comment.
I think this comment now applies to both methods since both methods take Status argument now.
| type, resource); | ||
| respTimer = null; | ||
| onAbsent(null, activeCpc.getServerInfo()); | ||
| respTimer = null; |
There was a problem hiding this comment.
Why swap the order of these statements?
There was a problem hiding this comment.
The original code was setting respTimer = null before calling onAbsent(), which caused the timeout context to be lost, resulting in a generic "does not exist" error.
The original for onAbsent method did not have a check for if (respTimer == null). It only looked at the serverInfo.resourceTimerIsTransientError() flag to decide between calling watcher.onError() or watcher.onResourceDoesNotExist(). It could not distinguish between a timeout and other "not found" conditions.
| logger.log(XdsLogLevel.WARNING, "No working fallback XDS Servers found from {0}", | ||
| activeCpClient.getServerInfo().target()); | ||
| activeCpc.getServerInfo().target()); | ||
| } |
There was a problem hiding this comment.
The changes above here seem unnecessary?
| executor.execute(() -> { | ||
| try { | ||
| notifyWatcher(watcher, data); | ||
| watcher.onResourceChanged(update); // Call the new method |
There was a problem hiding this comment.
Remove the comment as we are passing StatusOr argument.
| continue; | ||
| } | ||
| if (subscriber.hasResult()) { | ||
| subscriber.onError(status, null); // This will become an onAmbientError |
There was a problem hiding this comment.
As the handling to call either onDataChanged or onAmbientError happens in ResourceSubscriber.onError, why not simply call it? You are only calling if subscriber.hasResult() is true. What if when it is false? Previously the onError was getting called in that case but not now.
2 participants
Implements https://github.com/grpc/proposal/blob/master/A88-xds-data-error-handling.md#a88-xds-data-error-handling