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

api: enable selecting subset of services using rendezvous hashing #12862

Merged
merged 2 commits into from
Jun 30, 2022

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented May 3, 2022

This PR adds the ?choose query parameter to the /v1/service/<service> endpoint.

The value of choose is in the form <number>|<key>, number is the number
of desired services and key is a value unique but consistent to the requester
(e.g. allocID).

Folks aren't really expected to use this API directly, but rather through consul-template / template block
which will soon be getting a new helper function making use of this query parameter.

Example,

curl 'localhost:4646/v1/service/redis?choose=2|abc123'

Part of #12575

Because this also includes updating CT, the template part works now as well.

@mikenomitch
Copy link
Contributor

Ignorable nit: Should we split the number and the key into different params? And then require that both are set?

@shoenig shoenig marked this pull request as draft May 6, 2022 17:31
@shoenig shoenig force-pushed the f-choose-services branch 2 times, most recently from 671193b to c3ba29c Compare June 25, 2022 15:23
This PR adds the 'choose' query parameter to the '/v1/service/<service>' endpoint.

The value of 'choose' is in the form '<number>|<key>', number is the number
of desired services and key is a value unique but consistent to the requester
(e.g. allocID).

Folks aren't really expected to use this API directly, but rather through consul-template
which will soon be getting a new helper function making use of this query parameter.

Example,

curl 'localhost:4646/v1/service/redis?choose=2|abc123'

Note: consul-templte v0.29.1 includes the necessary nomadServices functionality.
@shoenig
Copy link
Member Author

shoenig commented Jun 27, 2022

Will add e2e test + complete example in the docs in a followup PR

Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

LGTM, this will be really useful! Minor inline question, but as mentioned, I feel I know the answer already.

.changelog/12862.txt Outdated Show resolved Hide resolved
Comment on lines +471 to +473
if err != nil {
return nil, structs.ErrMalformedChooseParameter
}
Copy link
Member

Choose a reason for hiding this comment

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

I think I already know the answer to this, but would it be worth logging the error content for cluster operators so this is available somewhere to lookup alongside the generic error return message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the only intended user is template/consul-template which will always be correct, though I could see the benefit for someone writing their own API client. If we do start logging RPC failures though we should probably do it at all the err points for consistency

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.3.x backport to 1.3.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants