Skip to content

Conversation

@easwars
Copy link
Contributor

@easwars easwars commented Aug 30, 2024

This PR ensures that the ADS stream level flow control functionality added recently is kept local to the xdsclient/transport package. It also simplifies the implementation in a number of ways.

Summary of changes:

  • Simplify the public watch API by having a func parameter instead of an interface to notify that local processing of the update is complete
  • Simplify the flow control implementation by not having to worry about the actual number of watchers that are yet to process the most recent update, and instead having a boolean that indicates whether or not the update has been processed by all watchers.
    • The actual work of tracking how many watchers are yet to process the most recent update is moved to the authority, which is the one that is actually invoking the watcher callbacks.
  • The reason for defining xdsresource.OnDoneFunc and making the watcher callbacks use that instead of a plan func() is to ease documenting the interface.
    • Willing to change this if deemed to cause more harm than good.

One of the main motivations for this change is the ongoing refactor to move ADS and LRS functionality out of the xDS transport type into separate types. With the flow control functionality becoming local to the transport package, it can easily be moved into the eventual type that will provide the ADS functionality.

#ads-stream-flow-control

RELEASE NOTES: none

@easwars easwars requested a review from dfawley August 30, 2024 22:07
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Aug 30, 2024
@easwars easwars added this to the 1.67 Release milestone Aug 30, 2024
@codecov
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 91.86992% with 10 lines in your changes missing coverage. Please review.

Project coverage is 81.86%. Comparing base (845f62c) to head (d221151).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
.../balancer/clusterresolver/resource_resolver_eds.go 57.14% 3 Missing ⚠️
xds/internal/resolver/watch_service.go 83.33% 3 Missing ⚠️
xds/internal/testutils/resource_watcher.go 66.66% 2 Missing ⚠️
.../balancer/clusterresolver/resource_resolver_dns.go 75.00% 1 Missing ⚠️
xds/internal/xdsclient/clientimpl_watchers.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7578      +/-   ##
==========================================
+ Coverage   81.76%   81.86%   +0.09%     
==========================================
  Files         361      361              
  Lines       27825    27814      -11     
==========================================
+ Hits        22752    22769      +17     
+ Misses       3866     3850      -16     
+ Partials     1207     1195      -12     
Files with missing lines Coverage Δ
...s/internal/balancer/cdsbalancer/cluster_watcher.go 100.00% <100.00%> (ø)
...ternal/balancer/clusterresolver/clusterresolver.go 72.12% <100.00%> (ø)
...rnal/balancer/clusterresolver/resource_resolver.go 93.63% <100.00%> (ø)
xds/internal/server/listener_wrapper.go 78.61% <100.00%> (ø)
xds/internal/server/rds_handler.go 86.76% <100.00%> (ø)
xds/internal/xdsclient/authority.go 89.20% <100.00%> (+0.20%) ⬆️
xds/internal/xdsclient/transport/transport.go 96.42% <100.00%> (+2.38%) ⬆️
...nal/xdsclient/xdsresource/cluster_resource_type.go 80.00% <100.00%> (ø)
...l/xdsclient/xdsresource/endpoints_resource_type.go 78.57% <100.00%> (ø)
...al/xdsclient/xdsresource/listener_resource_type.go 87.23% <100.00%> (ø)
... and 7 more

... and 29 files with indirect coverage changes

@dfawley dfawley assigned easwars and unassigned dfawley Sep 3, 2024
@easwars easwars merged commit 92111dc into grpc:master Sep 3, 2024
@easwars easwars deleted the xdsclient_ads_flow_control_simplification branch September 3, 2024 18:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants