-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Support "near" parameter in prepared query service block #2137
Changes from 7 commits
3797e65
865c264
100a467
4c1afb1
03fea4b
d567d6a
c457ee0
0c2ad07
104b234
62884a2
00819e8
7fd0c3c
01b28b9
46b6726
2b24644
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -368,10 +368,44 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, | |
// Shuffle the results in case coordinates are not available if they | ||
// requested an RTT sort. | ||
reply.Nodes.Shuffle() | ||
if err := p.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes); err != nil { | ||
|
||
// Get the source to sort by. This can be passed in by the requestor, or | ||
// pre-defined using the Near parameter in the prepared query. If the | ||
// near parameter was defined, that will be preferred. | ||
sortFrom := args.Source | ||
if query.Service.Near != "" { | ||
sortFrom = structs.QuerySource{ | ||
Datacenter: args.Datacenter, | ||
Node: query.Service.Near, | ||
} | ||
} | ||
|
||
// Respect the magic "_agent" flag. | ||
preferLocal := false | ||
if sortFrom.Node == "_agent" { | ||
preferLocal = true | ||
sortFrom = args.Origin | ||
} | ||
|
||
// Perform the distance sort | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to memoize this value for ~60s or whatever the interval is for the tomography calcs? I seem to recall someone saying this calc was "expensive," but if it's not, disregard. DNS can set a TTL, so maybe this isn't a huge issue, but that depends on the level of DNS caching at a given site. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not too bad - I'd probably leave as-is since this can be DNS TTL-ed and read-scaled with staleness across the servers. |
||
if err := p.srv.sortNodesByDistanceFrom(sortFrom, reply.Nodes); err != nil { | ||
return err | ||
} | ||
|
||
// Nodes cannot be any "closer" than localhost, so this special case ensures | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem worth the special case since 0.6 has been out for a while and nobody should be disabling coordinates :-) It'll do some stuff in the common case where the thing is already in the front position as well, and that would add extra complexity to avoid. |
||
// the local node is returned first if it is present in the result. This | ||
// allows the local agent to be preferred even when network coordinates are | ||
// not enabled. Only works if the results come from the request origin DC. | ||
if preferLocal && reply.Datacenter == args.Origin.Datacenter { | ||
for i, node := range reply.Nodes { | ||
if node.Node.Node == args.Origin.Node { | ||
remote := append(reply.Nodes[:i], reply.Nodes[i+1:]...) | ||
reply.Nodes = append([]structs.CheckServiceNode{node}, remote...) | ||
break | ||
} | ||
} | ||
} | ||
|
||
// Apply the limit if given. | ||
if args.Limit > 0 && len(reply.Nodes) > args.Limit { | ||
reply.Nodes = reply.Nodes[:args.Limit] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,11 @@ type ServiceQuery struct { | |
// discarded) | ||
OnlyPassing bool | ||
|
||
// Near allows the query to always prefer the node nearest the given | ||
// node. If the node does not exist, results are returned in their | ||
// normal randomly-shuffled order. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be good to mention the magic |
||
Near string | ||
|
||
// Tags are a set of required and/or disallowed tags. If a tag is in | ||
// this list it must be present. If the tag is preceded with "!" then | ||
// it is disallowed. | ||
|
@@ -173,6 +178,12 @@ type PreparedQueryExecuteRequest struct { | |
// Limit will trim the resulting list down to the given limit. | ||
Limit int | ||
|
||
// Origin is used to carry around a reference to the node which | ||
// is executing the query on behalf of the client. It is taken | ||
// as a QuerySource so that it can be used directly for queries | ||
// relating to the agent servicing the request. | ||
Origin QuerySource | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you take a look at the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty close yeah. I originally tried to use that, but I switched to this because in the HTTP/DNS api code at the agent, you can't be sure what the prepared query actually does, so the agent can't be sure if it should pass its own name + datacenter, or the user's provided |
||
|
||
// Source is used to sort the results relative to a given node using | ||
// network coordinates. | ||
Source QuerySource | ||
|
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 think this'll break other endpoints that depend on
?near=_agent
getting resolved at this level. I think you can pass the existingSource
member of the execute request into here.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.
Does it break? This just pushes the check out to the
consul
side instead of theagent
. I was trying to avoid checking the magic flag in the agent and on the consul side (since theNear
parameter can live in either place now).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.
Still think you'll need to revert this change - should not be needed any more and there are other RPC endpoints that do the sort which rely on
_agent
getting resolved on the agent side, not the server side.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.
Oh I see, catalog and health both allow distance sorting. Will fix!