xds: apply valid resources while NACKing update #8506
Conversation
ejona86
added
the
TODO:backport
| } | ||
|
|
||
| handleResourcesAccepted(ResourceType.CDS, parsedResources, versionInfo, nonce); | ||
| // CDS responses represents the state of the world, EDS resources not referenced in CDS |
There was a problem hiding this comment.
Why do we even need logic like this? If EDS is no longer needed then the subscriber will go away and we'll naturally stop watching it. I remember discussing this in the past, but maybe I don't remember the previous resolution.
There was a problem hiding this comment.
Yeah, we had a very long debate with @voidzcy one/two years ago. There was very subtle reason that convinced me to accept this approach, but I can't recall now.
I'm keeping this behavior in this PR to implement the change, but therefore I have to add code like https://github.com/grpc/grpc-java/pull/8506/files/5fe2a6bc3dd52f2687a91903732c443567c0e6b1#diff-6b019eb1d3518381d336168847890513a235ea4974fcb3c76a2e9ace43f1d460R2036-R2052 which makes ClientXdsClient so strongly coupled with LdsUpdate and CdsUpdate's deeply nested fields, or with LDS/CDS config's semantic.
| // Notify the watchers. | ||
| if (parsedResources.containsKey(resourceName)) { | ||
| subscriber.onData(parsedResources.get(resourceName), version, updateTime); | ||
| } else if (type == ResourceType.LDS || type == ResourceType.CDS) { | ||
| if (subscriber.data != null && invalidResources.contains(resourceName)) { |
There was a problem hiding this comment.
It seems this condition should move into the invalidResources.contains(resourceName) case above and then we combine the invalidResources.contains(resourceName into the if-else chain.
There was a problem hiding this comment.
I'm not sure how to simplify this. The subscriber.onAbsent() below still need (the negation of) this condition. I grouped the logic regarding retainedResources and onAbsent() together because they are related.
There was a problem hiding this comment.
I was thinking:
if (invalidResources.contains(resourceName)) {
subscriber.onRejected(version, updateTime, errorDetail);
if (subscriber.data != null) {
if (type == ResourceType.LDS) {
...
retainedResources.add(rdsName);
} else if (type == ResourceType.CDS) {
...
retainedResources.add(edsName);
}
}
} else if (parsedResources.containsKey(resourceName)) {
subscriber.onData(parsedResources.get(resourceName), version, updateTime);
} else if (type == ResourceType.LDS || type == ResourceType.CDS) {
subscriber.onAbsent();
}But I can live with what's here.
| // Notify the watchers. | ||
| if (parsedResources.containsKey(resourceName)) { | ||
| subscriber.onData(parsedResources.get(resourceName), version, updateTime); | ||
| } else if (type == ResourceType.LDS || type == ResourceType.CDS) { | ||
| if (subscriber.data != null && invalidResources.contains(resourceName)) { |
There was a problem hiding this comment.
I was thinking:
if (invalidResources.contains(resourceName)) {
subscriber.onRejected(version, updateTime, errorDetail);
if (subscriber.data != null) {
if (type == ResourceType.LDS) {
...
retainedResources.add(rdsName);
} else if (type == ResourceType.CDS) {
...
retainedResources.add(edsName);
}
}
} else if (parsedResources.containsKey(resourceName)) {
subscriber.onData(parsedResources.get(resourceName), version, updateTime);
} else if (type == ResourceType.LDS || type == ResourceType.CDS) {
subscriber.onAbsent();
}But I can live with what's here.
Implementing [gRFC A46](grpc/proposal#260)
dapengzhang0
removed
the
TODO:backport
Implementing [gRFC A46](grpc/proposal#260)
Implementing gRFC A46