xds: xdsClient caches transient error for new watchers

[shivaspeaks]

Fixes #11672

[kannanjgithub]

.

[ejona86]

With this change, I really want to kill errorDescription, as it is just lastError = Status.INVALID_ARGUMENT. However, some of the fallback code is using errorDescription in really weird ways, so we'd have to clean that up first.

[ejona86]

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.

[shivaspeaks]

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.

[ejona86]

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().

[kannanjgithub]

Unit tests?

[shivaspeaks]

Unit tests are needed for this? I don't know how I replicate this behaviour. The logic sounds well, we should probably let it merge and see?

[kannanjgithub]

Unit tests prevent breakages in the future from new code changes.
There are many tests that test onData and onError and testing watcher calls.

[ejona86]

A unit test shouldn't be too hard. We find a test that is triggering onError(), and then after getting the onError() add a second subscription.

So as "the first one I noticed" there's ldsResourceUpdated_withXdstpResourceName_withUnknownAuthority().

grpc-java/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java

Lines 680 to 695 in 6462ef9

	@Test
	public void ldsResourceUpdated_withXdstpResourceName_withUnknownAuthority() {
	String ldsResourceName =
	"xdstp://unknown.example.com/envoy.config.listener.v3.Listener/listener1";
	xdsClient.watchXdsResource(XdsListenerResource.getInstance(), ldsResourceName,
	ldsResourceWatcher);
	verify(ldsResourceWatcher).onError(errorCaptor.capture());
	Status error = errorCaptor.getValue();
	assertThat(error.getCode()).isEqualTo(Code.INVALID_ARGUMENT);
	assertThat(error.getDescription()).isEqualTo(
	"Wrong configuration: xds server does not exist for resource " + ldsResourceName);
	assertThat(resourceDiscoveryCalls.poll()).isNull();
	xdsClient.cancelXdsResourceWatch(XdsListenerResource.getInstance(), ldsResourceName,
	ldsResourceWatcher);
	assertThat(resourceDiscoveryCalls.poll()).isNull();
	}

You could create a ldsResourceWatcher2 and subscribe to the same resource after the verify(ldsResourceWatcher).onError(errorCaptor.capture());, and expect verify(ldsResourceWatcher2).onError(any());. You can also delete unimportant parts of the test, like checking the status description. You might want to verify that the second watcher gets the same Status, although that could even be optional.

[shivaspeaks]

I'm not confident with this unit test since we are not checking the instance to be same. It seems to be getting caught in errorDescription != null(code link) for both the watchers and not checking the savedError.

[shivaspeaks]

This PR introduces lastError, so there's no other test right now for setting lastError.
I tried moving check for savedError before errorDescription but looks like lastError is not getting set.

[kannanjgithub]

Your PR introduces lastError but the existing control paths in other tests do go there and set it. Just set breakpoint and run all tests in that test class and you will see.

[shivaspeaks]

Looks like none of the tests are stopping at that break point from GrpcXdsClientImplTestBase class.

[kannanjgithub]

[shivaspeaks]

Oh I was keeping the break point inside onError where savedError was getting defined. My bad. I made a new commit. PTAL.

Replied in comments