service: HealthCheckingLoadBalancerFactory using its own attribute - #6704
Conversation
|
|
||
| @Internal | ||
| @ResolutionResultAttr | ||
| public static final Attributes.Key<Map<String, ?>> RESOLUTION_RESULT_ATTR_HEALTH_CHECKING_CONFIG = |
There was a problem hiding this comment.
This should be in GrpcAttributes.java
There was a problem hiding this comment.
in current world yes, but the Attributes is actually API not an implementation of io.grpc.api (core). for example, if user depends on io.grpc.services 1.n.x, io.grpc.api 1.n.x and io.grpc.core 1.(n-1).x. it won't work. because, we are using the attributes as API. so, going forward the shared attributes should be in the api not in core. we are trying to fix this issue slowly from this change.
NOTE: this specific change requires latest version of all 3 artifacts
@ejona86 can explain it better (or correct my understanding).
There was a problem hiding this comment.
io.grpc.internal should be considered convenience. Easiest test to check if it is convenience is to ask if it would work if each user of it shaded it. This attribute is used cross-module, between a transport/core and a load balancer, so it should not be in internal.
The same would be true of GrpcAttributes.ATTR_LB_PROVIDED_BACKEND. ATTR_CLIENT_EAG_ATTRS is a hack that maybe should be moved out as well, but maybe we want to replace it with something else. So that just leaves ATTR_SECURITY_LEVEL, which is sort of fine, but could be done in a more explicit way; it was sort of a lazy solution to pass data out of transports.
So in general, GrpcAttributes is a code smell and any new attributes added there should be treated with suspicion.
| @SuppressWarnings("unchecked") | ||
| Map<String, ?> verifiedRawServiceConfig = (Map<String, ?>) rawServiceConfig.getConfig(); | ||
| Map<String, ?> healthCheckedService = | ||
| ServiceConfigUtil.getHealthCheckedService(verifiedRawServiceConfig); |
There was a problem hiding this comment.
This doesn't look right. I think we want this to be in ManangedChannelImpl so that it applies to all name resolvers.
What made me notice this is that RESOLUTION_RESULT_ATTR_HEALTH_CHECKING_CONFIG was put in NameResolver and not LoadBalancer. I expected it in LoadBalancer and for the NameResolver to be ignorant of it.
There was a problem hiding this comment.
moved to LoadBalancer, PTAL
voidzcy
left a comment
voidzcy
left a comment
There was a problem hiding this comment.
Btw, would you need a test case in ManagedChannelImpl to cover HealthCheckingCoonfig gets parsed and propagated?
yes, now it is testable. i'll create some. I wish i could test the attribute, but ManagedChannelImplTest is faking that part. |
| private static Map<String, Object> parseConfig(String json) throws Exception { | ||
| return (Map<String, Object>) JsonParser.parse(json); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
nit: new line.
Btw, why not just put in ManagedChannelImplTest.java?
There was a problem hiding this comment.
because this test is only testing ManagedChannelServiceConfig. i would love to verify the MCI sets the new Attribute or not but the MCI test is faking that particular code path.
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
I think ideally we would just save the health name String (basically by calling ServiceConfigUtil.getHealthCheckedServiceName()) within ManagedChannelImpl, but this is a good step in the right direction and fixes the immediate problem.
3 participants
prerequisite for removing
NAME_RESOLVERSERVICE_CONFIGattribute.