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

Use roundRobin balancing even if only one hostname #1329

Merged
merged 4 commits into from
Feb 13, 2019

Conversation

benley
Copy link
Contributor

@benley benley commented Feb 12, 2019

Which problem is this PR solving?

  • When using GRPC between jaeger-agent and jaeger-collector, one must currently enumerate each backend on the commandline in order to get any load balancing. GRPC can do better with its pluggable resolver modules.

Short description of the changes

  • If you run jaeger-agent with --reporter.grpc.host-port dns:///jaeger-collector-hostname:14250 (note the dns:/// part!), the GRPC client resolves the given hostname to one or more IPs and can load balance among the backend addresses it finds. It even looks up and obeys SRV records, if they exist. This change makes jaeger-agent use round-robin load balancing in that situation, rather than the default of pick_first. If there is only one backend, or the user doesn't explicitly put dns:/// before the hostname, this will have no effect.

See https://www.marwan.io/blog/grpc-dns-load-balancing for more details.

Question: should this dns resolver style be the default behaviour? That can be done easily as well.

If you run jaeger-agent with `--reporter.grpc.host-port
dns:///jaeger-collector-hostname:14250` (note the `dns:///` part!),
the GRPC client resolves the given hostname to one or more IPs and can
load balance among the backend addresses it finds.  It even looks up
and obeys SRV records, if they exist.  This change makes jaeger-agent
use round-robin load balancing in that situation, rather than the
default of `pick_first`.  If there is only one backend, or the user
doesn't explicitly put `dns:///` before the hostname, this will
have no effect.

Signed-off-by: Benjamin Staffin <benley@gmail.com>
@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #1329 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1329   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         162     162           
  Lines        7295    7295           
======================================
  Hits         7295    7295
Impacted Files Coverage Δ
cmd/agent/app/reporter/grpc/collector_proxy.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8023bd6...e595c30. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

TIL

@jpkrohling
Copy link
Contributor

In time: you'd get bonus points if you could add this to the documentation and/or to the help options.

@benley
Copy link
Contributor Author

benley commented Feb 12, 2019

In time: you'd get bonus points if you could add this to the documentation and/or to the help options.

How does this look? jaegertracing/documentation#212

@ghost ghost assigned yurishkuro Feb 12, 2019
@ghost ghost added the review label Feb 12, 2019
@yurishkuro
Copy link
Member

@benley awesome!

did you see any tickets that might be closed by this? I'm pretty sure we had these discussions.

Also, would you like to add Postmates to #207 / ADOPTERS.md ?

@benley
Copy link
Contributor Author

benley commented Feb 12, 2019

did you see any tickets that might be closed by this?

#213 is closely related but I'm not sure if this really closes it. We're adding some much-needed functionality for grpc balancing but DNS service discovery isn't going to be as reliable as, say, Zookeeper or etcd or Kubernetes API watches for noticing when the set of backends changes.

Also, would you like to add Postmates to #207 / ADOPTERS.md ?

Sure, I will do that!

@yurishkuro yurishkuro merged commit 7c0ed8c into jaegertracing:master Feb 13, 2019
@ghost ghost removed the review label Feb 13, 2019
@benley benley deleted the grpc-dns-balance branch February 13, 2019 21:41
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
If you run jaeger-agent with `--reporter.grpc.host-port
dns:///jaeger-collector-hostname:14250` (note the `dns:///` part!),
the GRPC client resolves the given hostname to one or more IPs and can
load balance among the backend addresses it finds.  It even looks up
and obeys SRV records, if they exist.  This change makes jaeger-agent
use round-robin load balancing in that situation, rather than the
default of `pick_first`.  If there is only one backend, or the user
doesn't explicitly put `dns:///` before the hostname, this will
have no effect.

Signed-off-by: Benjamin Staffin <benley@gmail.com>
Signed-off-by: Iori Yoneji <fivo.11235813@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
If you run jaeger-agent with `--reporter.grpc.host-port
dns:///jaeger-collector-hostname:14250` (note the `dns:///` part!),
the GRPC client resolves the given hostname to one or more IPs and can
load balance among the backend addresses it finds.  It even looks up
and obeys SRV records, if they exist.  This change makes jaeger-agent
use round-robin load balancing in that situation, rather than the
default of `pick_first`.  If there is only one backend, or the user
doesn't explicitly put `dns:///` before the hostname, this will
have no effect.

Signed-off-by: Benjamin Staffin <benley@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
If you run jaeger-agent with `--reporter.grpc.host-port
dns:///jaeger-collector-hostname:14250` (note the `dns:///` part!),
the GRPC client resolves the given hostname to one or more IPs and can
load balance among the backend addresses it finds.  It even looks up
and obeys SRV records, if they exist.  This change makes jaeger-agent
use round-robin load balancing in that situation, rather than the
default of `pick_first`.  If there is only one backend, or the user
doesn't explicitly put `dns:///` before the hostname, this will
have no effect.

Signed-off-by: Benjamin Staffin <benley@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
If you run jaeger-agent with `--reporter.grpc.host-port
dns:///jaeger-collector-hostname:14250` (note the `dns:///` part!),
the GRPC client resolves the given hostname to one or more IPs and can
load balance among the backend addresses it finds.  It even looks up
and obeys SRV records, if they exist.  This change makes jaeger-agent
use round-robin load balancing in that situation, rather than the
default of `pick_first`.  If there is only one backend, or the user
doesn't explicitly put `dns:///` before the hostname, this will
have no effect.

Signed-off-by: Benjamin Staffin <benley@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants