Skip to content

Conversation

vinothkumarr227
Copy link
Contributor

@vinothkumarr227 vinothkumarr227 commented Sep 16, 2025

Fixes: #8381

RELEASE NOTES:

  • xds/internal: Refactored the gRPC xdsclient to eliminate temporary wrappers and embeddings, directly aligning its resource types and watchers with the generic xdsclient interfaces

Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 79.02439% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.00%. Comparing base (b36320e) to head (c17c44c).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
...xds/xdsclient/xdsresource/cluster_resource_type.go 80.00% 7 Missing and 3 partials ⚠️
...s/xdsclient/xdsresource/endpoints_resource_type.go 70.58% 7 Missing and 3 partials ⚠️
...ds/xdsclient/xdsresource/listener_resource_type.go 79.59% 7 Missing and 3 partials ⚠️
...dsclient/xdsresource/route_config_resource_type.go 70.58% 7 Missing and 3 partials ⚠️
internal/xds/server/listener_wrapper.go 60.00% 2 Missing ⚠️
internal/xds/testutils/fakeclient/client.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8582      +/-   ##
==========================================
+ Coverage   81.98%   82.00%   +0.02%     
==========================================
  Files         413      415       +2     
  Lines       40528    40738     +210     
==========================================
+ Hits        33225    33407     +182     
+ Misses       5944     5934      -10     
- Partials     1359     1397      +38     
Files with missing lines Coverage Δ
...ternal/xds/balancer/cdsbalancer/cluster_watcher.go 100.00% <100.00%> (ø)
internal/xds/balancer/clusterimpl/clusterimpl.go 85.01% <100.00%> (-1.05%) ⬇️
.../balancer/clusterresolver/resource_resolver_eds.go 67.24% <100.00%> (-6.45%) ⬇️
...ernal/xds/balancer/loadstore/load_store_wrapper.go 74.00% <100.00%> (ø)
internal/xds/resolver/watch_service.go 100.00% <100.00%> (ø)
internal/xds/server/rds_handler.go 80.00% <100.00%> (+4.71%) ⬆️
internal/xds/xdsclient/client.go 100.00% <ø> (ø)
internal/xds/xdsclient/clientimpl.go 82.20% <100.00%> (ø)
internal/xds/xdsclient/clientimpl_loadreport.go 76.92% <100.00%> (ø)
internal/xds/xdsclient/clientimpl_watchers.go 100.00% <100.00%> (ø)
... and 9 more

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vinothkumarr227 vinothkumarr227 changed the title xds/internal:Removed Generic resource Decoder and added concrete functions #8381 xds/internal:remove Generic resource Decoder and added concrete functions #8381 Sep 16, 2025
@eshitachandwani eshitachandwani self-assigned this Sep 17, 2025
@eshitachandwani eshitachandwani added this to the 1.77 Release milestone Sep 17, 2025
@eshitachandwani eshitachandwani added Type: Internal Cleanup Refactors, etc Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Sep 17, 2025
@eshitachandwani eshitachandwani self-requested a review September 18, 2025 07:04
@eshitachandwani
Copy link
Member

@vinothkumarr227 , can you please break this PR down into smaller PRs with one resource in one PR so that it is easier to understand and review.


// LoadStore is the minimal interface returned by ReportLoad. It provides a
// way to get per-cluster reporters and to stop the store.
type LoadStore interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we add this interface?

// balancer when LRS is enabled. Before that, all functions to record loads
// are no-op.
store *lrsclient.LoadStore
perCluster *lrsclient.PerClusterReporter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we change it to use the grpc xdsclient interfaces that we created?

// are are deserialized and validated, as received from the xDS management
// server. Upon receipt of a response from the management server, an
// appropriate callback on the watcher is invoked.
func (c *clientImpl) WatchResource(rType xdsresource.Type, resourceName string, watcher xdsresource.ResourceWatcher) (cancel func()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are changing the type parameter, can we change it to a string so that the signatures match and we can directly call the xdsclient.WatchResource?

// interface with ref counting so that it can be shared by the xds resolver and
// balancer implementations, across multiple ClientConns and Servers.
type clientImpl struct {
*xdsclient.XDSClient // TODO: #8313 - get rid of embedding, if possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we make this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: xDS Includes everything xDS related, including LB policies used with xDS. Type: Internal Cleanup Refactors, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xds/internal: Remove wrappers and embeddings from grpc xdsclient and re-write resource types and watchers as per generic client interfaces
2 participants