search

uber-archive / hyperbahn

Service discovery and routing for large scale microservice operations
MIT License
395 stars 57 forks source link

Bail out of egress Hyperbahn::discover calls for non-affine services #207

Closed kriskowal closed 8 years ago

kriskowal commented 8 years ago

r @Raynos @jcorbin @thanodnl

kriskowal commented 8 years ago

This is a first stab at this issue. I’m using the "cn" header in this PR, but if we elect to "fake" the CN with the original service to create a per-origin circuit breaker on the Hyperbahn channel, this will not work.

kriskowal commented 8 years ago

We could alternately add a property to the IDL to distinguish a forwarded request. Not sure how Ringpop’s handle-or-forward resolved the issue cc @thanodnl

thanodnl commented 8 years ago

@kriskowal HandleOrForward only 'solves' it when you do not overwrite the endpoint your self. HandleOrForwards defaults to forward to the /proxy/req endpoint on the ringpop channel. And the handler there does not forward anything. It is a really hard api to use correctly.

How we lately solved this in ringpop-go is by setting a ringpop-forwarded in the application level header. The call is then sent to ring member that owns the key. Here a check is done to see if it is a forwarded call and if so it always executes it locally.

Lately I have been thinking that the node it is forwarded to should also check it believes it is the owner of the shardkey, if not send an error response back. But that is something that we are not really sure about how to handle at the moment. And it might be application specific what needs to be done. Some applications do not work if they execute things on the wrong machines, some applications have degraded performance so it will probably be a configuration option for application owners to specify.

Raynos commented 8 years ago

I think the correct solution is two methods

discover() and discoverAffinity()

Raynos commented 8 years ago

Let's get this in and let's get a hotpatch build ready just in case.

Raynos commented 8 years ago

Merging.

kriskowal commented 8 years ago

Sounds good. We can make a better fix in a follow-up.