Skip to content

Conversation

vinothkumarr227
Copy link
Contributor

@vinothkumarr227 vinothkumarr227 commented Oct 15, 2025

Addresses: #8381

RELEASE NOTES: None

Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 76.85185% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.52%. Comparing base (ae62635) to head (d4221a6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...xds/xdsclient/xdsresource/cluster_resource_type.go 71.42% 5 Missing and 5 partials ⚠️
...s/xdsclient/xdsresource/endpoints_resource_type.go 80.76% 3 Missing and 2 partials ⚠️
...dsclient/xdsresource/route_config_resource_type.go 80.76% 3 Missing and 2 partials ⚠️
...ternal/xds/balancer/cdsbalancer/cluster_watcher.go 33.33% 1 Missing and 1 partial ⚠️
internal/xds/resolver/xds_resolver.go 33.33% 0 Missing and 2 partials ⚠️
internal/xds/balancer/cdsbalancer/cdsbalancer.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8652      +/-   ##
==========================================
+ Coverage   81.21%   81.52%   +0.30%     
==========================================
  Files         416      416              
  Lines       41002    40921      -81     
==========================================
+ Hits        33298    33359      +61     
+ Misses       6226     6167      -59     
+ Partials     1478     1395      -83     
Files with missing lines Coverage Δ
.../balancer/clusterresolver/resource_resolver_eds.go 73.68% <100.00%> (+7.01%) ⬆️
internal/xds/resolver/watch_service.go 100.00% <100.00%> (+2.85%) ⬆️
internal/xds/server/listener_wrapper.go 60.58% <ø> (+7.11%) ⬆️
internal/xds/server/rds_handler.go 75.28% <100.00%> (ø)
internal/xds/xdsclient/client.go 50.00% <ø> (ø)
internal/xds/xdsclient/clientimpl_watchers.go 40.00% <100.00%> (-20.00%) ⬇️
internal/xds/xdsclient/resource_types.go 100.00% <100.00%> (ø)
...ds/xdsclient/xdsresource/listener_resource_type.go 95.23% <100.00%> (+48.29%) ⬆️
...nternal/xds/xdsclient/xdsresource/resource_type.go 0.00% <ø> (-21.74%) ⬇️
internal/xds/balancer/cdsbalancer/cdsbalancer.go 83.68% <50.00%> (-0.24%) ⬇️
... and 5 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 removed their assignment Oct 16, 2025
@easwars easwars self-assigned this Oct 16, 2025
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Just some minor nits this time around.

Comment on lines 33 to 35
// Producer watches resources via the external xdsclient API.
type Producer interface {
// WatchResource uses xDS to discover the resource associated with the
// provided resource name. The resource type implementation determines how
// xDS responses 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.
WatchResource(rType Type, resourceName string, watcher ResourceWatcher) (cancel func())
}

// ProducerV2 is like Producer, but uses the external xdsclient API.
//
// Once all resource type implementations have been migrated to use the external
// xdsclient API, this interface will be renamed to Producer and the existing
// Producer interface will be deleted.
type ProducerV2 interface {
WatchResourceV2(typeURL, resourceName string, watcher xdsclient.ResourceWatcher) (cancel func())
WatchResource(typeURL, resourceName string, watcher xdsclient.ResourceWatcher) (cancel func())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please retain the old comments of the Producer interface. The fact that this interface is using the external xdsclient API is not very important here. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

r.applyRouteConfigUpdate(update)
r.applyRouteConfigUpdate(*update)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change applyRouteConfigUpdate to accept a pointer and change the currentRouteConfig field to hold a pointer as well. That we won't have to deref the pointer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// the watcher is canceled. Callers need to handle this case.
WatchResource(rType xdsresource.Type, resourceName string, watcher xdsresource.ResourceWatcher) (cancel func())

// WatchResourceV2 matches the API of the external xdsclient interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to retain the old docstring for WatchResource. That contains a bunch of useful information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@easwars easwars assigned vinothkumarr227 and unassigned easwars Oct 16, 2025
@vinothkumarr227 vinothkumarr227 removed their assignment Oct 17, 2025
@dfawley dfawley changed the title xds/internal:remove Generic resource Decoder and added concrete functions xds/internal: remove Generic resource Decoder and add concrete functions Oct 17, 2025
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. Status: Requires Reporter Clarification Type: Internal Cleanup Refactors, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants