-
Notifications
You must be signed in to change notification settings - Fork 20
Add support for prepared queries #50
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
Conversation
c59c254 to
c1b786d
Compare
|
Can you force-push again to pass the unit tests? |
This adds support for prepared queries destinations in addition to service ones: an upstream can now target a prepared query and benefit from features such as fallback between DCs. Since prepared queries do not support bloquing requests this uses polling to periodically check for changes. The default polling rate is 30s: the same as the one consul uses, and can be overriden with the "poll_interval" config option.
c1b786d to
e45d5f7
Compare
pierresouchay
left a comment
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 am Ok, implementation sounds good, but I would like to have exponential backoff in case of failure and ability to tune the prepared query interval if possible
| }) | ||
| if err != nil { | ||
| w.log.Errorf("consul: error fetching service definition for service %s: %s", up.DestinationName, err) | ||
| time.Sleep(errorWaitTime) |
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.
Can you do a truncated binary exponential backoff instead of a fixed one ?
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 can, however, that's how errors are handled in all the other watches. I can make this a separate PR.
| last = nodesP | ||
| } | ||
|
|
||
| time.Sleep(interval) |
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 am Ok with 30s as a default duration, but could you make it configurable ?
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.
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.
Yes, sorry
pierresouchay
left a comment
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.
OK for me, but exponential back off seems important to me, but OK in another PR
This adds support for prepared queries destinations in addition to
service ones: an upstream can now target a prepared query and benefit
from features such as fallback between DCs.
Since prepared queries do not support bloquing requests this uses polling
to periodically check for changes. The default polling rate is 30s: the
same as the one consul uses, and can be overriden with the "poll_interval"
config option.