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 Throughput Loadbalancer (Looking for feedback) #314

Closed
wants to merge 8 commits into from
Closed

gRPC Throughput Loadbalancer (Looking for feedback) #314

wants to merge 8 commits into from

Conversation

bradylove
Copy link
Contributor

@bradylove bradylove commented Sep 5, 2017

This is not yet ready to merge, some investigation/work still needs to be done on the gRPC Throughput Load Balancer

This PR updates the pool to use the new gRPC Throughput Load Balancer.

The gRPC Throughput Load Balancer will grow/shrink (shrinking not yet implemented) the number of connections to Doppler as the number of concurrent requests increase. Currently each connection to Doppler is allowed 100 concurrent RPC/Stream requests. This number will need some testing and tweaking.

//cc @apoydence @enocom @jasonkeene

@cfdreddbot
Copy link

Hey bradylove!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/150809680

The labels on this github issue will be updated when the story is started.

@jasonkeene
Copy link
Contributor

jasonkeene commented Sep 7, 2017

  • grpc-throughputlb reads weird, perhaps grpc-throughput-lb ?
  • 7 Days ago a new load balancer API was introduced to grpc-go (see gRFC L9, and source). We should try using this instead if that is the future.

I'll give more feedback as things evolve.

@bradylove
Copy link
Contributor Author

@jasonkeene I opted for @grpc-throughputlb so that throughputlb would match the package name without having any special characters in it.

Once I finish this load balancer I will look at updating it to the new API. That balancer interface is not yet usable but I will keep an eye on it.

@enocom
Copy link
Contributor

enocom commented Sep 12, 2017

Another thing worth looking at is the grpc-go document on load balancing.

@bradylove
Copy link
Contributor Author

This has been updated with a simplified load balancing strategy by removing the elasticity of connections. Now the load balancer will open a given number of connections on start and then evenly balance the load between all of those connections.

[submodule "src/code.cloudfoundry.org/grpc-throughputlb"]
path = src/code.cloudfoundry.org/grpc-throughputlb
url = https://github.com/cloudfoundry-incubator/grpc-throughputlb
[submodule "src/github.com/alecthomas/gometalinter"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose leaving tooling-related concerns out of our submodules, and instead add a step to install any tools to our test script.

Copy link
Contributor Author

@bradylove bradylove Oct 16, 2017

Choose a reason for hiding this comment

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

That got pulled in during a conflict while rebasing. Thought it was added by loggregator. I will remove.

@bradylove
Copy link
Contributor Author

I have squashed the commits which appears to have broken the PR. I am going to reopen for final review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants