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

grpc-js: Big refactor in preparation for implementing retries #2243

Merged
merged 5 commits into from Oct 14, 2022

Conversation

murgatroid99
Copy link
Member

This PR includes a few related changes:

First, the Http2CallStream type is split into 3 composed classes: the ResolvingCall class is responsible for tracking call deadlines and for handling call configs from the name resolver. Once it gets a call config, it creates and delegates to a LoadBalancingCall. The LoadBalancingCall class is responsible for invoking filters, getting headers from call credentials, calculating timeouts from deadlines, and handling pick results from the the LB policy. Once it gets a successful pick, it creates and delegates to a SubchannelCall, which owns and manages a ClientHttp2Stream and performs most of the work that Http2CallStream was originally responsible for. The Subchannel now has a createCall method that returns a SubchannelCall, which replaces the startCallStream method that attached a new HTTP2 stream to an existing Http2CallStream object.

Second, the InnerChannel class contains the implementation logic for the channel and methods for other classes in the library to interact with, while the Channel class contains the public channel API and delegates to the InnerChannel for all logic. This change and the previous one are both strongly based on the architecture of the C core.

Third, pickers from LB policies can no longer provide filters. Instead, they can provide onCallStarted and onCallEnded callbacks to track when individual calls start and end, including the status they end with.

In addition, there are a variety of incidental changes. With responsibility for deadlines and call credentials moved out of filters, the filter behavior can be restricted, and they are no longer constructed with a reference to the call. Also, in a few instances, types, functions, and classes that were originally used in only one file are now used in multiple files, so they have been moved into their own separate files.


The first change is the most important one that facilitates adding the retry feature. The goal is to add a RetryingCall class that ResolvingCall can use in place of LoadBalancingCall, and RetryingCall will use LoadBalancingCall to represent each attempt. In addition, the progress field that LoadBalancingCall adds to status objects provides information that RetryingCall will need to make decisions about retries.

I also added retry configs to the service config type definitions. Those will be processed by RetryingCall.

@gnossen
Copy link

gnossen commented Oct 12, 2022

This is in my review queue. I'll look into it tomorrow.

@gnossen
Copy link

gnossen commented Oct 13, 2022

First of all, thank you very much for the detailed PR description. Way to break the trend of highly complex code changes with no prose explanation whatsoever.

Copy link

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

I will fully admit that I did not read through the full implementation deeply enough to understand this change thoroughly. However:

  • The structure of the change looks solid.
  • It looks like no actual test changes were required.

So count me as an LGTM as long as the tests are green and non-flaky.

@murgatroid99
Copy link
Member Author

One error in an interop test with the old library, rerunning: https://source.cloud.google.com/results/invocations/779a9381-9c21-4f05-b36f-8de5c1b2ba92/targets

@murgatroid99 murgatroid99 merged commit 3e0a037 into grpc:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants