xds: Parsing xDS Cluster Metadata#11741
Conversation
|
Note: I will add unit tests in the next commit after finalizing on main code. Please react on this comment (👍) if this looks good, and then I will proceed with unit tests. |
| return INSTANCE; | ||
| } | ||
|
|
||
| ClusterMetadataValueParser findParser(String typeUrl) { |
There was a problem hiding this comment.
This should have a @Nullable annotation as the underlying get(key) method could return null if the key (typeUrl) is not found in the HashMap
There was a problem hiding this comment.
@Nullable is not required in grpc-java. It is inconsistent, and without a null checker, it is nothing more than documentation.
| .build(); | ||
|
|
||
| Struct filterMetadata = Struct.newBuilder() | ||
| .putFields("key1", Value.newBuilder().setStringValue("value1").build()) |
There was a problem hiding this comment.
Should we craft other types of Values in the test cases to excersise all branches of the convertValue method above (e.g. LIST_VALUE, BOOL_VALUE, etc.)?
There was a problem hiding this comment.
I don't feel like doing so right now, as we are concerned only about Audience here which will always be StringValue. We definitely need to add tests as and when the usage grows. Adding more tests on different value types right now will be purely based on assumption.
There was a problem hiding this comment.
we are concerned only about Audience here which will always be StringValue ... Adding more tests on different value types right now will be purely based on assumption.
If we not using the other cases and only using STRING_VALUE, then we should delete the other unused branches in convertValue until they are needed. This would keep the code and tests focused.
There was a problem hiding this comment.
Independent of the plan, I suspect we shouldn't do that sort of testing here. It can be part of a separate ProtobufJsonConverterTest.
Deleting the branches in ProtobufJsonConverter doesn't seem to benefit anyone.
There was a problem hiding this comment.
We are not only concerned with Audience here. You are doing two things: 1) adding the metadata infrastructure and 2) supporting Audience. Properly handling the JSON is part of (1). We should add a ProtobufJsonConverterTest to fully-cover the converter. This should be very easy.
danielzhaotongliu
left a comment
danielzhaotongliu
left a comment
There was a problem hiding this comment.
nit: the PR title should start with xds: (module name) as it is all xDS related changes per convertValue per https://github.com/grpc/grpc-java/blob/master/CONTRIBUTING.md#guidelines-for-pull-requests
| .build(); | ||
|
|
||
| Struct filterMetadata = Struct.newBuilder() | ||
| .putFields("key1", Value.newBuilder().setStringValue("value1").build()) |
There was a problem hiding this comment.
we are concerned only about Audience here which will always be StringValue ... Adding more tests on different value types right now will be purely based on assumption.
If we not using the other cases and only using STRING_VALUE, then we should delete the other unused branches in convertValue until they are needed. This would keep the code and tests focused.
shivaspeaks
changed the title
| * <p>This class maintains a mapping of type URLs to {@link ClusterMetadataValueParser} instances, | ||
| * allowing for the parsing of different metadata types. | ||
| */ | ||
| final class ClusterMetadataRegistry { |
There was a problem hiding this comment.
This shouldn't have "Cluster" in its name. It is for all xds metadata.
3 participants
Add Support for Audience Metadata Parsing in xDS Cluster Resource