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

Name resolution load balancing #1015

Merged
merged 17 commits into from
Sep 24, 2019

Conversation

murgatroid99
Copy link
Member

This adds a custom name resolver and load balancer internal API with basic implementations. The name resolver implementation handles retrieving and service config data from DNS TXT records, but most of the service config options are not implemented. The only implemented load balancer implementation is "pick first". It will attempt to establish connections to every address returned by the resolver, and will use whichever one connects first.

This change fixes the following bugs:

Some parts of this design are not in this PR and will be added in future PRs:

  • UDS resolver
  • Round robin load balancer
  • Channel argument handling for the features implemented here

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 29, 2019

CLA Check
The committers are authorized under a signed CLA.

);
},
};
// TODO: check channel arg for default service config
Copy link
Member

Choose a reason for hiding this comment

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

Add your username to the TODO

@@ -138,7 +138,9 @@ class UnknownHandler extends CompressionHandler {
compressMessage(message: Buffer): Promise<Buffer> {
return Promise.reject<Buffer>(
new Error(
`Received message compressed wth unsupported compression method ${this.compressionName}`
`Received message compressed wth unsupported compression method ${
Copy link
Member

Choose a reason for hiding this comment

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

That's a bit of a weird looking change. What's the reasoning?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this came from the code formatter.

Copy link
Contributor

Choose a reason for hiding this comment

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

There’s a typo in wth — it should be with.

import * as resolver from './resolver';
import * as load_balancer from './load-balancer';

function setup() {
Copy link
Member

Choose a reason for hiding this comment

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

The typical paradigm for these is rather something along these lines:

(function() {
  resolver.registerAll();
  load_balancer.registerAll();
}());

@felipegcampos
Copy link

Good job on these bugs. I think this PR might fix 2 problems we're currently experiencing. Do you have any estimations on when it should be released?

@murgatroid99
Copy link
Member Author

@felipegcampos We want to get it out soon, but it's a lot of code so it's going to take some time for it to be reviewed.

@felipegcampos
Copy link

@felipegcampos We want to get it out soon, but it's a lot of code so it's going to take some time for it to be reviewed.

Perfect. I was reviewing myself. However, I don't know if I'm of much help since I have not been checking the codebase since we started with grpc. However, let me know if I can help somehow.

@murgatroid99 murgatroid99 merged commit e055468 into grpc:master Sep 24, 2019
cjihrig added a commit to cjihrig/grpc-node that referenced this pull request Sep 25, 2019
This commit fixes a typo from
grpc#1015
cjihrig added a commit to cjihrig/grpc-node that referenced this pull request Sep 25, 2019
This commit fixes a typo observed in
grpc#1015
@cjihrig cjihrig mentioned this pull request Sep 25, 2019
@murgatroid99 murgatroid99 mentioned this pull request Sep 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 23, 2019
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.

5 participants