xds: add "resource_timer_is_transient_failure" server feature

[shivaspeaks]

As per gRFC A88 (grpc/proposal#466).

[ejona86]

We can just call onError(). That's equivalent for the current API as there is no data yet.

[shivaspeaks]

I see a lot of new code has been merged since then. There are a lot of new failures in files like CdsLoadBalancer2Test, GrpcXdsClientImplTestBase, GrpcXdsClientImplV3Test, etc.
Same files and lines were also getting changed in xds client watcher API changes. Perhaps this should wait until xds client watcher api changes goes in?

[ejona86]

Why should there be any new failures? Sounds like you aren't setting resourceTimerIsTransientError correctly to preserve the old behavior for the majority of the tests.

[kannanjgithub]

Unit test for watcher.onError and the timeoutSec value when resource timer is transient error.

[kannanjgithub]

typo: CUSTOM_AUTHORITY

[kannanjgithub]

We could delete this statement.

[shivaspeaks]

I was doing that exactly earlier but after every test method there is an @After function that runs:

@After
  public void tearDown() {
    XdsClusterResource.enableLeastRequest = originalEnableLeastRequest;
    xdsClient.shutdown();
    channel.shutdown();  // channel not owned by XdsClient
    assertThat(adsEnded.get()).isTrue();
    assertThat(lrsEnded.get()).isTrue();
    assertThat(fakeClock.getPendingTasks()).isEmpty();
  }

This fails while asserting for empty pending tasks. We need to run all the due tasks at the end.

[ejona86]

This is used by CSDS, and looks like it crashes if you don't update it:

grpc-java/xds/src/main/java/io/grpc/xds/CsdsService.java

Lines 240 to 255 in 5a8326f

	static ClientResourceStatus metadataStatusToClientStatus(ResourceMetadataStatus status) {
	switch (status) {
	case UNKNOWN:
	return ClientResourceStatus.UNKNOWN;
	case DOES_NOT_EXIST:
	return ClientResourceStatus.DOES_NOT_EXIST;
	case REQUESTED:
	return ClientResourceStatus.REQUESTED;
	case ACKED:
	return ClientResourceStatus.ACKED;
	case NACKED:
	return ClientResourceStatus.NACKED;
	default:
	throw new AssertionError("Unexpected ResourceMetadataStatus: " + status);
	}
	}

[shivaspeaks]

So in the case of TIMEOUT, CSDS service should return either ClientResourceStatus.REQUESTED or ClientResourceStatus.DOES_NOT_EXIST. What do you think? DOES_NOT_EXIST fits better.

[ejona86]

That is clearly not something for us to arbitrarily decide during implementation. That's defined by the gRFC. They map 1:1 with the CSDS enum values.

Those states map directly to the states in the CSDS ClientResourceStatus enum.

[shivaspeaks]

Right. I see now: envoyproxy/envoy@2aaa544. We need to do envoy proto sync to grpc java to have these.

[ejona86]

Checking xdsDataErrorHandlingEnabled doesn't seem to do anything, as the flag protecting is happening in BootstrapperImpl.

[ejona86]

Instead of putting this on the entire method, can you put it on just the ResourceWatcher<CdsUpdate> timeoutWatcher definition? Ditto for resourceTimerIsTransientError_schedulesExtendedTimeout.