Skip to content

Conversation

@eshitachandwani
Copy link
Member

This is part of A74 implementation which add CDS/EDS/DNS watchers to the dependency manager. It also adds a temporary flag that is disabled by default so that it is not used in the current RPC paths , but enabled in the dependency manager tests.

RELEASE NOTES: None

@eshitachandwani eshitachandwani added this to the 1.78 Release milestone Dec 4, 2025
@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 80.96591% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.34%. Comparing base (6ed8acb) to head (cd5d990).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
internal/xds/xdsdepmgr/xds_dependency_manager.go 80.80% 50 Missing and 17 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8744      +/-   ##
==========================================
- Coverage   83.39%   83.34%   -0.05%     
==========================================
  Files         419      418       -1     
  Lines       32566    32897     +331     
==========================================
+ Hits        27159    27419     +260     
- Misses       4023     4084      +61     
- Partials     1384     1394      +10     
Files with missing lines Coverage Δ
internal/xds/resolver/xds_resolver.go 88.76% <100.00%> (+0.12%) ⬆️
internal/xds/xdsdepmgr/xds_dependency_manager.go 80.87% <80.80%> (-4.04%) ⬇️

... and 22 files with indirect coverage changes

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

@easwars
Copy link
Contributor

easwars commented Dec 11, 2025

Can you also please check the codecov report here: #8744 (comment) to see if there are any valid lines that are missing test coverage. Thanks.

@eshitachandwani eshitachandwani removed their assignment Dec 16, 2025
Comment on lines 204 to 216
{ID: clients.Locality{
Region: "region-1",
Zone: "zone-1",
SubZone: "subzone-1",
},
Endpoints: []xdsresource.Endpoint{
{
Addresses: []string{addr},
HealthStatus: xdsresource.EndpointHealthStatusUnknown,
Weight: 1,
},
},
Weight: 1},
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to be fixed yet.

Comment on lines 727 to 739
{ID: clients.Locality{
Region: "region-1",
Zone: "zone-1",
SubZone: "subzone-1",
},
Endpoints: []xdsresource.Endpoint{
{
Addresses: []string{"localhost:8081"},
HealthStatus: xdsresource.EndpointHealthStatusUnknown,
Weight: 1,
},
},
Weight: 1},
Copy link
Contributor

Choose a reason for hiding this comment

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

Hasn't been fixed yet.

@easwars
Copy link
Contributor

easwars commented Dec 17, 2025

I made a small refactoring commit to split the populateClusterConfigLocked method into smaller ones to handle updates for specific cluster types.

I also added the default case for the switch at the end. Please note that I changed it return true, nil, nil instead of false, nil, nil as it was doing earlier, because in this case, we have received the cluster resource. It just happens to be a type that we don't support. That shouldn't block us from sending an update if everything else in the tree is resolved (since we are anyways storing the error in the cluster config).

@easwars easwars assigned eshitachandwani and unassigned easwars Dec 17, 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.

Some of my comments might be outdated as this point, as I pushed a couple of commits for more generics and some simplification around the dns resolver.

@easwars easwars assigned eshitachandwani and unassigned easwars Dec 18, 2025
@easwars easwars removed their assignment Dec 19, 2025
@eshitachandwani eshitachandwani merged commit 4046676 into grpc:master Dec 19, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants