xds: xdsClient caches transient error for new watchers #12262
Conversation
| @@ -894,6 +903,9 @@ void onError(Status error, @Nullable ProcessingTracker tracker) { | |||
| Status errorAugmented = Status.fromCode(error.getCode()) | |||
| .withDescription(description + "nodeID: " + bootstrapInfo.node().getId()) | |||
| .withCause(error.getCause()); | |||
| this.lastError = errorAugmented; | |||
| this.data = null; | |||
There was a problem hiding this comment.
Cached data should continue to get used when available, until we implement A88 onResourceChanged
| @@ -894,6 +903,9 @@ void onError(Status error, @Nullable ProcessingTracker tracker) { | |||
| Status errorAugmented = Status.fromCode(error.getCode()) | |||
| .withDescription(description + "nodeID: " + bootstrapInfo.node().getId()) | |||
| .withCause(error.getCause()); | |||
| this.lastError = errorAugmented; | |||
| this.data = null; | |||
| this.absent = false; | |||
There was a problem hiding this comment.
We should not change this, only a onData arrival should reset this to false as there is reading logic using this value on onAbsent.
| @@ -676,6 +676,8 @@ private final class ResourceSubscriber<T extends ResourceUpdate> { | |||
| private ResourceMetadata metadata; | |||
| @Nullable | |||
| private String errorDescription; | |||
| @Nullable | |||
| private Status lastError; | |||
There was a problem hiding this comment.
FYI: Change data to StatusOr<T> data would have kept this a bit more clear. You still have data vs absent, but at least you don't have yet another thing to deal with.
There was a problem hiding this comment.
I'm concerned that it's a more invasive refactoring change since it would require touching every method that uses the data field. I can try to do it in a follow up PR and keep this PR's scope limited to bug fix?
I'll agree that we are moving to more usage of StatusOr<T> in our codebase everywhere. We should consider here as well for consistency.
There was a problem hiding this comment.
There's only a few other places that access data. It does look like it'd make all of them more complicated. Most are of the form data != null, and would likely need to become data != null && data.hasValue().
Fixes #11672