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.

Sign up for GitHub

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

Jump to bottom

Add support for datacenters, tags and filtering to Consul #3281

Merged
merged 5 commits into from
Jan 27, 2021
Merged

Add support for datacenters, tags and filtering to Consul #3281

merged 5 commits into from
Jan 27, 2021

Conversation

renaudb
Copy link
Contributor

@renaudb renaudb commented Jan 13, 2021

This adds support for three Consul features:

  • Creating Consul EndpointGroups that point to different datacenters.
  • Filtering of Consul Endpoints using builtin Consul filters.
  • Adding tags to Consul services on registration.

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@renaudb
Copy link
Contributor Author

renaudb commented Jan 13, 2021

This is my first contribution, so I'm not familiar with the process yet. Let me know if I should tag this as a draft.

ikhoon
ikhoon reviewed Jan 13, 2021
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks for your first contribution! 🙏 I think this PR does not need a draft tag.

consul/src/main/java/com/linecorp/armeria/client/consul/ConsulEndpointGroupBuilder.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/client/consul/ConsulEndpointGroupBuilder.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/internal/consul/CatalogClient.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/server/consul/ConsulUpdatingListenerBuilder.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/server/consul/ConsulUpdatingListenerBuilder.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/server/consul/ConsulUpdatingListenerBuilder.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/server/consul/ConsulUpdatingListenerBuilder.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/internal/consul/AgentServiceClient.java Outdated Show resolved Hide resolved
@minwoox minwoox added this to the 1.4.0 milestone Jan 13, 2021
@codecov
Copy link

codecov bot commented Jan 13, 2021 •
edited
Loading

Codecov Report

Merging #3281 (0b6a396) into master (a4724be) will decrease coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3281      +/-   ##
============================================
- Coverage     74.06%   74.05%   -0.02%     
- Complexity    13008    13134     +126     
============================================
  Files          1136     1143       +7     
  Lines         49276    49764     +488     
  Branches       6256     6347      +91     
============================================
+ Hits          36496    36852     +356     
- Misses         9566     9663      +97     
- Partials       3214     3249      +35
Impacted Files Coverage Δ Complexity Δ
...a/server/consul/ConsulUpdatingListenerBuilder.java 84.84% <66.66%> (-4.05%) 11.00 <1.00> (+1.00) ⬇️
...orp/armeria/client/consul/ConsulEndpointGroup.java 74.28% <75.00%> (+10.64%) 8.00 <0.00> (+2.00)
...rp/armeria/internal/consul/AgentServiceClient.java 63.33% <75.00%> (-0.96%) 4.00 <1.00> (ø)
...eria/client/consul/ConsulEndpointGroupBuilder.java 76.00% <100.00%> (+4.57%) 8.00 <2.00> (+2.00)
...inecorp/armeria/internal/consul/CatalogClient.java 50.00% <100.00%> (+5.10%) 9.00 <0.00> (+1.00)
...linecorp/armeria/internal/consul/ConsulClient.java 100.00% <100.00%> (ø) 13.00 <5.00> (+2.00)
...corp/armeria/internal/consul/ConsulClientUtil.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
...linecorp/armeria/internal/consul/HealthClient.java 60.97% <100.00%> (+3.08%) 6.00 <0.00> (ø)
.../armeria/server/consul/ConsulUpdatingListener.java 58.20% <100.00%> (+0.63%) 12.00 <2.00> (ø)
...inecorp/armeria/common/ClosedSessionException.java 54.54% <0.00%> (-18.19%) 5.00% <0.00%> (-1.00%)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4724be...0b6a396. Read the comment docs.

@renaudb renaudb requested a review from ikhoon January 14, 2021 03:07
minwoox
minwoox reviewed Jan 15, 2021
View reviewed changes
consul/src/main/java/com/linecorp/armeria/internal/consul/HealthClient.java Outdated Show resolved Hide resolved
consul/src/main/java/com/linecorp/armeria/server/consul/ConsulUpdatingListenerBuilder.java
@@ -157,6 +180,6 @@ public ConsulUpdatingListenerBuilder consulToken(String consulToken) {
*/
public ConsulUpdatingListener build() {
return new ConsulUpdatingListener(consulClientBuilder.build(), serviceName, serviceEndpoint,
checkUri, checkMethod, checkInterval);
checkUri, checkMethod, checkInterval, tagsBuilder.build().asList());
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use Set for the tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible if we change the type of Service.tags to List<String>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, the Consul API allows for duplicate tags, since it is passed as a JSON list of strings. In practice though, it's unlikely that this is the user's desired behaviour (see issue below).

hashicorp/consul#2329

I kept Service.tags as a List<String> to match the Consul API, in case a service returned by Consul does in fact contains duplicate tags (although Armeria does not currently contains any read call to the service API). However, I kept it a Set<String> here, since as I mentioned, I don't expect users to voluntarily want to add the same tags multiple times.

So I guess there are 3 options:

  1. Follow the Consul API strictly and make everything a List<String>, potentially letting users add the same tag multiple time by mistake.

  2. Go against the Consul API and make everything a Set<String>, potentially hiding duplicate tags on service reads call (although there are no such calls currently in the Armeria codebase).

  3. Keep the current approach of having a public API that only allows a Set<String> of tags to reflect the most common user intent, but internally use a List<String> to match the Consul API.

Let me know which one is preferable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the kind words. I didn't know the detailed specification. In that case, I lean toward option 3.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. 😄 Yeah, I'm fine as it is.

@renaudb renaudb requested a review from minwoox January 15, 2021 17:00
ikhoon
ikhoon approved these changes Jan 18, 2021
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @renaudb 🙏

minwoox
minwoox approved these changes Jan 18, 2021
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @renaudb!
Hope to see you more. 😄

trustin
trustin reviewed Jan 20, 2021
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Just one nit. It's awesome to see your pull request, @renaudb!

consul/src/main/java/com/linecorp/armeria/server/consul/ConsulUpdatingListenerBuilder.java Outdated Show resolved Hide resolved
@renaudb
Copy link
Contributor Author

renaudb commented Jan 25, 2021

The test failures in AppVeyor seem unrelated?

@ikhoon
Copy link
Contributor

ikhoon commented Jan 25, 2021

The test failures in AppVeyor seem unrelated?

Yes. It is a flaky test.
Please ignore TomcatServiceDestroyedConnectorTest.serviceUnavailableAfterConnectorIsDestroyed()

@minwoox
Copy link
Member

minwoox commented Jan 25, 2021

I have fixed that in #3217 but it's not merged yet. Sorry about it. 😅

trustin
trustin approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks, @renaudb !

@trustin trustin merged commit 6198d1f into line:master Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@trustin trustin trustin approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@renaudb @CLAassistant @ikhoon @minwoox @trustin