Implement Hash-Based Routing#519
Conversation
7c44bfe to
2c23efd
Compare
2c23efd to
91f5968
Compare
src/code.cloudfoundry.org/gorouter/proxy/round_tripper/proxy_round_tripper.go
Outdated
Show resolved
Hide resolved
e9cbe0c to
7b456f8
Compare
9f21f6b to
513c575
Compare
peanball
left a comment
There was a problem hiding this comment.
First set of comments. I have not looked at the implementation of maglev and the hash based instance determination.
I've focussed on the integration with the existing Gorouter code, route lookup, LB algo handling, etc.
| if e != nil { | ||
| e.RLock() | ||
| defer e.RUnlock() | ||
| return e.endpoint |
There was a problem hiding this comment.
e might not be valid anymore, as it is retrieved without lock. You then acquire a lock and should re-fetch the value to be sure that it's still there.
This commit provides the basic implementation for hash-based routing. It does not consider the balance factor yet. Co-authored-by: Clemens Hoffmann <clemens.hoffmann@sap.com> Co-authored-by: Tamara Boehm <tamara.boehm@sap.com> Co-authored-by: Soha Alboghdady <soha.alboghdady@sap.com>
5030bbc to
560f935
Compare
peanball
left a comment
There was a problem hiding this comment.
Some more comments, now on the hash_based.go
|
LGTM. Once we have the performance tests, we should move to community review. |
|
Good news - we evaluated the performance of gorouter with hash-based routing support. Our test setup constists of scaled out HAProxy (as a load balancer in front of gorouter), scaled out Diego Cells and a very simple and performant application with 30 instances. This allows us to performance test gorouter via external requests, where gorouter is the bottleneck. We compared the performance of the latest stable routing-release, and the same release including changes from this Pull-Request. Using
We also explored that requests are routed to next instances as expected, when the max. connection limit of one app instance was reached. The hash_balance factor is effective and allows distribution of requests to next instances before overloading it. Please feel free to reach out for details on the results! |
Changes were addressed. As this is still subject to change, I cannot give "final" approval yet.
21cc8b7 to
b414a38
Compare
|
During performance tests, we figured out that the Gorouter memory consumption is higher than expected if we keep the permutation table (table which is used to fill the final maglev lookup table) in memory. We want to investigate this finding further and optimize it. |
Co-authored-by: Alexander Nicke <alexander.nicke@sap.com>
hoffmaen
left a comment
There was a problem hiding this comment.
Only documentation / log related comments.
Hello, I am requesting approver status for the Networking group. My contributions so far: Pull-Requests: cloudfoundry/gorouter#442 cloudfoundry/gorouter#435 cloudfoundry/routing-release#519 cloudfoundry/routing-release#514 cloudfoundry/routing-release#504 cloudfoundry/routing-release#453 cloudfoundry/routing-release#434 cloudfoundry/cli#3378 cloudfoundry/cloud_controller_ng#4080 cloudfoundry/cloud_controller_ng#4199 Co-Authorship: cloudfoundry/routing-release#478 Pull-Request Reviews: cloudfoundry/gorouter#443 cloudfoundry/routing-release#452 cloudfoundry/cli#3372 GitHub Issues: cloudfoundry/routing-release#529 cloudfoundry/routing-release#468 cloudfoundry/routing-release#445 cloudfoundry/routing-release#429 cloudfoundry/cloud_controller_ng#4198
Summary
This Pull-Request implements #505.
Backward Compatibility
Breaking Change? Yes/No