-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
examples/features/loadbalancing: Add custom lb example #6649
Conversation
|
||
# custom_round_robin | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a similar explanation of this case, keeping a similar structure, rather than just starting with output text. "The third client is configured to use a custom ............"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -84,11 +89,24 @@ func main() { | |||
|
|||
fmt.Println("--- calling helloworld.Greeter/SayHello with round_robin ---") | |||
makeRPCs(roundrobinConn, 10) | |||
// You can also plug in your own custom lb policy, which needs to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a blank line above this for better visual separation please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// configurable. This n is configurable. Try changing it and see how the | ||
// behavior changes. | ||
customroundrobinConn, err := grpc.Dial( | ||
fmt.Sprintf("%s:///%s", exampleScheme, exampleServiceName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pull this out at the top of the function:
target := fmt.Sprintf("%s:///%s", exampleScheme, exampleServiceName)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Good point.
serviceconfig.LoadBalancingConfig `json:"-"` | ||
|
||
// N represents how often pick iterations chose the second SubConn | ||
// in the list. Defaults to 3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch. Switched the default in ParseConfig() to 3.
func (customRoundRobinBuilder) Build(cc balancer.ClientConn, bOpts balancer.BuildOptions) balancer.Balancer { | ||
crr := &customRoundRobin{} | ||
baseBuilder := base.NewBalancerBuilder(customRRName, crr, base.Config{HealthCheck: true}) | ||
crr.Balancer = baseBuilder.Build(cc, bOpts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not a fan of the base balancer...are we sure we want users using it as a template for how to do their own LB policy? I think it would be better to show the actual connection management APIs here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to petiole policy wrapping pick first after offline discussion.
} | ||
|
||
// Following is an example name resolver implementation. Read the name | ||
// resolution example to learn more about it. | ||
|
||
type exampleResolverBuilder struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this example intends to show off custom resolvers - can this be replaced with the manual resolver instead? We may not have had it when this was created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched.
@@ -123,4 +141,87 @@ func (*exampleResolver) Close() {} | |||
|
|||
func init() { | |||
resolver.Register(&exampleResolverBuilder{}) | |||
balancer.Register(&customRoundRobinBuilder{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put the custom LB policy into its own file (or even its own sub-package), since that's how we expect it to be used typically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
func (crr *customRoundRobin) Build(info base.PickerBuildInfo) balancer.Picker { | ||
if len(info.ReadySCs) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to make this deterministic, then also do this until len(info.ReadySCs) == 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made deterministic in new PR based off child balancer connectivity states.
Opened a new PR #6691 containing all the offline discussion decisions such as making Custom Load Balancer a petiole policy wrapping pick_first load balancers. Made a whole new example because this example is outdated and shows concepts like custom resolvers, and new example needs to use endpoints. |
This PR adds a custom lb example, with a custom lb deployed as the top level balancer of the channel through service config. This scales up a previous example
RELEASE NOTES: