Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor AdsClient and switch backoff with tryhard #430

Merged
merged 3 commits into from
Nov 3, 2021

Conversation

XAMPPRocky
Copy link
Collaborator

While looking at investigating how our code handles shutdown signals and seeing how we could improve it, and the code in the AdsClient was particularly gnarly to understand, so I've split up the responsibilities of the AdsClient into distinct types to split the code up, similar to how it was done in #339. Now instead of all the operations being associated functions on AdsClient, the responsiblities are split up between RpcSender, RpcReceiver, RpcSession, and AdsClient. Like #339 there's no actual behaviour change here.

  • RpcSender is responsible for sending discovery requests.
  • RpcReceiver is responsible for handling discovery responses.
  • RpcSession encapsulates both RpcSender and RpcReceiver into a single session with a server.
  • AdsClient handles RpcSession initiation, failure, and retrying.

This PR also replaces using backoff for the handling backoff on failure with tryhard. The motivation being that tryhard is a lot simpler conceptually and in implementation than backoff, with backoff we had to control the backoff ourselves, and pass state back and forth to ensure we can properly retry. Where as now with tryhard we don't have to do any of that, we just run the future, and that is all taken of for us.

@XAMPPRocky XAMPPRocky added the kind/cleanup Refactoring code, fixing up documentation, etc label Nov 2, 2021
@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@github-actions github-actions bot added the size/l label Nov 2, 2021
})
.with_config(retry_config);

tokio::select! {
Copy link
Member

Choose a reason for hiding this comment

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

I'll defer the majority of the review to @iffyio who knows this code section far better than I - but I will definitely say that this section is far more clear 👍🏻

src/xds/ads_client.rs Show resolved Hide resolved
src/xds/cluster.rs Show resolved Hide resolved
@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 3b269d60-5e0e-4085-b685-e7e39b5e62dd

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/430/head:pr_430 && git checkout pr_430
cargo build

@markmandel markmandel merged commit 1653ec4 into main Nov 3, 2021
@markmandel markmandel deleted the replace-backoff-with-tryhard branch November 18, 2021 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kind/cleanup Refactoring code, fixing up documentation, etc size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants