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

Add Coordinate endpoint #67

Merged
merged 3 commits into from
Feb 4, 2022
Merged

Conversation

indev29
Copy link
Contributor

@indev29 indev29 commented Feb 3, 2022

This PR implements coordinate endpoint queries:

  • /coordinate/datacenters
  • /coordinate/nodes
  • /coordinate/node/:node

It doesn't add an implementation for /coordinate/update.

/coordinate/node/:node

Returning std::vector

I don't quite like returning a vector of nodes in /coordinate/node/:node, but the endpoint returns an array for this request (see Sample Response)

In Consul Enterprise, this may include multiple coordinates for the same node, each marked with a different Segment. Coordinates are only compatible within the same segment.

I'm not sure how ppconsul handles Enterprise features. It's not required in my use-case so it's up to you.
Should we leave it like that (i.e. return a vector)?

NotFoundError handling

Currently /coordinate/node/:node throws NotFoundError if node does not exist (because of the API responding with 404).
Should we catch it and return empty vector instead?

/agent/self

This request also returns Coordinate block, should we implement it aswell?
Although it seems to be difficult without breaking existing interface. Maybe by adding coordinates to Member struct?

@oliora
Copy link
Owner

oliora commented Feb 3, 2022

Hi Maxim, thank you for the PR! See my comments below.

Returning std::vector

Vector is fine

NotFoundError handling

Yeah, I don't like exception here either. If I need to design Ppconsul this days then I would return an empty optional but it's already designed and to keep consistency with other APIs let's return an empty vector.

/agent/self

Adding coordinates to /agent/self would be great if you have time for it. Please do It as a separate PR. Speaking of implementation I think it's better to break compatibility and drop pair<Config, Member> in favor of struct SelfInfo with fields config, member and coord (the new one).

@oliora
Copy link
Owner

oliora commented Feb 3, 2022

And one more thing. Several endpoints support a special value _all for segment parameter. It makes sense to add character constant All_Segments = "_all" to consul.h.

Comment on lines 31 to 37
return std::none_of(
nodes.begin(), nodes.end(),
[dim](const auto& node)
{
return (node.coord.vec.size() != dim);
}
);
Copy link
Owner

Choose a reason for hiding this comment

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

To remove the double negation (it's hard to understand) use std::all_of with "positive" predicate.

@oliora
Copy link
Owner

oliora commented Feb 3, 2022

If you need to see hot to handle status code without exceptions check Kv::item in kv.h

@indev29 indev29 requested a review from oliora February 4, 2022 06:32
@oliora oliora merged commit c8ff3b0 into oliora:master Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants