xds: remove HealthStatus field from xdsresource.Endpoint struct#9023
xds: remove HealthStatus field from xdsresource.Endpoint struct#9023chengxilo wants to merge 7 commits intogrpc:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9023 +/- ##
==========================================
- Coverage 83.01% 82.95% -0.06%
==========================================
Files 411 411
Lines 32960 32969 +9
==========================================
- Hits 27361 27350 -11
- Misses 4196 4211 +15
- Partials 1403 1408 +5
🚀 New features to boost your workflow:
|
Pranjali-2501
left a comment
There was a problem hiding this comment.
@chengxilo , Thanks for your PR.
|
solved😊 |
Pranjali-2501
left a comment
There was a problem hiding this comment.
LGTM.
Assigning it to @arjan-bal for second review.
| hs := xdsresource.HealthStatus(endpoint.ResolverEndpoint) | ||
| if hs != xdsresource.EndpointHealthStatusHealthy && hs != xdsresource.EndpointHealthStatusUnknown { |
There was a problem hiding this comment.
I'm not entirely convinced that moving the health status to attributes is beneficial. I have a few concerns:
- Unlike the hashkey and weight fields, health status is not currently propagated to LB policies via Endpoint attributes, so moving them doesn't result in memory savings or reduced code complexity.
- Attributes are designed for untyped data. Moving these fields requires type erasure during insertion and type assertions during extraction, which seems unnecessary. The existing
Endpointstruct provides better compile-time safety by avoiding this overhead.
@easwars, regarding #8757, what is the primary motivation for removing the Endpoint struct? Is that benefit significant enough to justify the loss of type safety when passing these values through internal packages?
There was a problem hiding this comment.
Upon further thought, I agree that it does not make sense to move things like health status into attributes because they are not plumbed down the LB policy tree. The same applies to the current Weight field in the Endpoint struct, because that is used only by the CDS policy to determine the eventual weight of the Endpoint which is then set as an attribute for LB policies like pick_first and ring_hash to consume. So, at the moment, we are left with HealthStatus and Weight fields which are not plumbed down the LB policy tree and therefore don't make sense to be carried as attributes. And Metadata is complicated to be carried in an attribute.
@chengxilo : I appreciate your efforts on attempting to fix these issues, and I apologize for not having given enough thought before filing them and marking them as "Help Wanted". Hope you can contribute to our repo through other open issues.
…9049) Fixes #8757 See #9023 (comment) for more context. RELEASE NOTES: none
Part of : #8757
This PR removes the HealthStatus field from the xdsresource.Endpoint struct and adds it as an attribute while un-marshalling EDS instead of setting it as an attribute later on in cluster resolver balancer
RELEASE NOTES: None