Consul prepared queries #1389
Consul prepared queries #1389
Conversation
|
We decided query-specific TTLs aren't needed since we have session integration, so took that off for now. |
| if err := structs.Decode(buf, &req); err != nil { | ||
| panic(fmt.Errorf("failed to decode request: %v", err)) | ||
| } | ||
| defer metrics.MeasureSince([]string{"consul", "fsm", "query", string(req.Op)}, time.Now()) |
There was a problem hiding this comment.
Can we change "query" to "prepared-query" to avoid confusion?
slackpad
force-pushed
the
f-prepared-queries
branch
7 times, most recently
from
f90cc4e to
4a15584
Compare
|
There's a TODO in there about using the new limit capability. It didn't pan out like I'd hoped because I forgot about the de-duping we do down in the DNS server. We could de-dup up in the query, or we could just set the limit to something "pretty large" for UDP-based queries which should work ok in practice (gives a reasonable cut of nodes to de-dup if you've got thousands of nodes to choose from). Right now I just don't set the limit so that it's equivalent to what we had before. |
slackpad
force-pushed
the
f-prepared-queries
branch
from
fbfda65 to
bc0c899
Compare
slackpad
force-pushed
the
f-prepared-queries
branch
from
bc0c899 to
e1ce1a3
Compare
|
Why did the |
|
@armon For the Sort option, with the failover options also using RTT it seemed too confusing/crazy UX-wise. I plumbed it as a Source parameter to the endpoint so it works like all the others with a It seems very tricky to use via DNS properly (no load balancing and the agent you are sorting based on can be unclear if you are proxying Consul's DNS, recursing, etc.) so WAN-only use of RTT seemed like the clear winner and less of a foot shooter. I'm definitely open to adding it back in the query definition if you think it's useful for DNS as well. It would be a minor change, might even be doable as an agent option. It might even be clearer as an agent option so it can be explained as "if I serve a prepared query, sort the results relative to me". |
| @@ -0,0 +1,175 @@ | |||
| package api | |||
|
|
|||
| import () | |||
There was a problem hiding this comment.
We can remove this since it is empty
|
@slackpad looking good, I left some minor feedback and questions throughout. |
|
Alright - I've addressed all the comments. Lemme know if you guys have any more concerns before I merge. |
|
🚀 🚀 🚀 |
This isn't quite ready for a full review but I thought I'd post it so you guys can take a quick look to see where it's going. Here's a todo list:
Most of these should be pretty quick once the first item is checked off. I'm going to rebase the last commit on there / force push, though the state store changes and the other commits up to the WIP one should be complete.