WIP Cloud Map Service Discovery #551
Conversation
| // TODO filter on health? | ||
| return aws.listInstances(listInstancesRequest).getInstances().stream() | ||
| .map(summary -> getInstance(serviceId, summary.getId())) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Currently making a bunch of sequential HTTP requests, any suggested alternatives?
Is it reasonable to pull in reactor here?
There was a problem hiding this comment.
potentially, there's also ReactiveDiscoveryClient
There was a problem hiding this comment.
Added a reactive client in fdffdf1.
| @Override | ||
| public List<ServiceInstance> getInstances(String serviceId) { | ||
| ListInstancesRequest listInstancesRequest = new ListInstancesRequest() | ||
| .withServiceId(serviceId); |
There was a problem hiding this comment.
Should we always be filtering for healthy instances only, or is that generally a configurable option?
There was a problem hiding this comment.
Could be an option or also could go in a filter for instance in ribbon or spring cloud loadbalancer
There was a problem hiding this comment.
ServiceInstance doesn't appear to have a health property, would that mean it would just need to look it up as a string via getMetadata?
spring-projects-issues
added
the
status: waiting-for-triage
|
Any particular implementations I should take a look at for testing inspiration? |
| // TODO filter on health? | ||
| return aws.listInstances(listInstancesRequest).getInstances().stream() | ||
| .map(summary -> getInstance(serviceId, summary.getId())) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
potentially, there's also ReactiveDiscoveryClient
| @Override | ||
| public List<ServiceInstance> getInstances(String serviceId) { | ||
| ListInstancesRequest listInstancesRequest = new ListInstancesRequest() | ||
| .withServiceId(serviceId); |
There was a problem hiding this comment.
Could be an option or also could go in a filter for instance in ribbon or spring cloud loadbalancer
|
|
||
| @Override | ||
| public boolean isSecure() { | ||
| return getPort() == 443; |
There was a problem hiding this comment.
Is this always the case?
There was a problem hiding this comment.
That's more of a best guess at the moment:
$ aws servicediscovery list-instances --service-id=srv-blah
{
"Instances": [
{
"Id": "fitz-test-instance",
"Attributes": {
"AWS_INSTANCE_IPV4": "blah",
"AWS_INSTANCE_IPV6": "blah blah blah",
"AWS_INSTANCE_PORT": "8080"
}
}
]
}
|
Got some of the configuration boilerplate knocked out, not sure what's next/what kind of testing makes sense for this |
| ListInstancesRequest listInstancesRequest = new ListInstancesRequest().withServiceId(serviceId); | ||
| // TODO pagination | ||
| // TODO parallel requests? | ||
| // TODO filter on health? | ||
| return aws.listInstances(listInstancesRequest).getInstances().stream() | ||
| .map(summary -> getInstance(serviceId, summary.getId())).collect(Collectors.toList()); |
There was a problem hiding this comment.
I'd recommend using the DiscoverInstances API that is built for this use-case.
- No need for pagination
- Filter on the health status is included out of the box
https://docs.aws.amazon.com/cloud-map/latest/api/API_DiscoverInstances.html
Super janky minimal implementation of an AWS Cloud Map based Service Discovery client (#398).
Currently meant as more of a conversation starter than something ready for a full review.
Note that @darylrobbins has a more fleshed out version of this parked in a branch, but I didn't feel comfortable opening a PR with his code.