xds: Fix XDS control plane client retry timer backoff duration when connection closes after results are received by larry-safran · Pull Request #11766 · grpc/grpc-java · GitHub
Skip to content

xds: Fix XDS control plane client retry timer backoff duration when connection closes after results are received#11766

Merged
larry-safran merged 2 commits into
grpc:masterfrom
larry-safran:fallback
Dec 19, 2024
Merged

xds: Fix XDS control plane client retry timer backoff duration when connection closes after results are received#11766
larry-safran merged 2 commits into
grpc:masterfrom
larry-safran:fallback

Conversation

@larry-safran

@larry-safran larry-safran commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

The backoffPolicy was calculating time since last attempt, but if the stream had been successfully connected and received responses it should delay from the current time (i.e. reset the stopwatch). Changed test so that stopwatch wasn't always 0, which masked the problem.

@larry-safran larry-safran requested a review from ejona86 December 19, 2024 19:38
@ejona86

ejona86 commented Dec 19, 2024

Copy link
Copy Markdown
Member

@ejona86

ejona86 commented Dec 19, 2024

Copy link
Copy Markdown
Member

Nit: include xds: in commit/PR titles when appropriate

@ejona86

ejona86 commented Dec 19, 2024

Copy link
Copy Markdown
Member

And in this case the title should also say "xds client" in some fashion. The title is not clear as-is.

@larry-safran larry-safran changed the title Fix retry timer backoff duration. xds: Fix retry timer backoff duration. Dec 19, 2024
@ejona86

ejona86 commented Dec 19, 2024

Copy link
Copy Markdown
Member

@danielzhaotongliu

@danielzhaotongliu danielzhaotongliu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Quick question for my understanding, StopWatch itself is not a thread-safe class and requires external synchronization, but in the control plane client it is thread safe because we run it in the synchronization context?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes. We rely heavily on synchronization context.

@larry-safran larry-safran changed the title xds: Fix retry timer backoff duration. xds: Fix XDS control plane client retry timer backoff duration when connection closes after results are received Dec 19, 2024
@larry-safran larry-safran merged commit ef7c2d5 into grpc:master Dec 19, 2024
@larry-safran larry-safran deleted the fallback branch December 20, 2024 18:29
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants