Added round_robin balancer as an option to gRPC client settings #1353
Conversation
RashmiRam
requested review from
bogdandrutu,
dmitryax,
flands,
james-bebbington,
owais,
pjanotti,
rghetia and
tigrannajaryan
as code owners
|
There are only 2 available balancers @bogdandrutu Let me know your thoughts too. |
config/configgrpc/configgrpc.go
Outdated
|
|
||
| // Sets the balancer as RoundRobin in grpclb_policy to discover the servers. Default is pick_first | ||
| // https://github.com/grpc/grpc-go/blob/master/examples/features/load_balancing/README.md | ||
| useRoundRobin bool `mapstructure:"useRoundRobin"` |
There was a problem hiding this comment.
I would use a string here instead of book, just in case other strategies will be supported. Same behavior, if not set use the first pick. What do you think?
There was a problem hiding this comment.
Changed it from bool to string and validated the same as setting invalid balancer name causes panic.
|
@RashmiRam I read your comment after I added mine. I still think having a string is better because gRPC may support other policies in the future and the problem with the boolean will hit us even if the user cannot create custom lb policies. |
…w balancers in future Setting invalid balancer is panicking. Hence validated the same & thrown an error
Codecov Report
@@ Coverage Diff @@
## master #1353 +/- ##
=======================================
Coverage 90.14% 90.14%
=======================================
Files 218 218
Lines 15256 15266 +10
=======================================
+ Hits 13752 13762 +10
Misses 1095 1095
Partials 409 409
Continue to review full report at Codecov.
|
|
The lint error seen in build failure is because of the usage of deprecated How do we go about this? Shall we make use of the |
|
I would go with that ugly API for the moment. |
config/configgrpc/configgrpc.go
Outdated
| return opts, nil | ||
| } | ||
|
|
||
| func validateBalancerName(balancerName string) (err error) { |
There was a problem hiding this comment.
Can we explicitly check here for the right values. I know we will need work when new format is accepted but that way we control our backwards compatible story
There was a problem hiding this comment.
Sorry. I couldn't get what you mean by backwards compatible story? Do you mean the API change for setting the balancerName?
There was a problem hiding this comment.
So, I ended up removing the other call to grpc.WithBalancerName in validateBalancerName to fix the lint errors.
open-telemetry#1353) Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Description:
Allow gRPC client settings like balancerName to be configured via config file.
Link to tracking Issue:
#1041
Testing:
No new tests are added. Added this option also in
TestAllGrpcClientSettingstestDocumentation:
Updated the README.md of jaegerexporter, opencensusexporter, otlpexporter to reflect the newly added option.
Closes #1041