-
Notifications
You must be signed in to change notification settings - Fork 169
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 SmallRye Stork service discovery #82
Conversation
@aureamunoz / @michalszynkiewicz this supercedes #68. Something got messed up there when I tried to rebase |
@aureamunoz you can see here I updated the |
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.
LGTM
return Uni.createFrom().completionStage(this::getRandomVillain) | ||
.onFailure().retry().withBackOff(Duration.ofMillis(200)).atMost(3); | ||
} | ||
private static final String STORK_PREFIX = "stork://"; |
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'm not sure at the moment, but I think using stork://
in the web target should also work (and would keep this code simpler).
Have you tried it?
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 didn't try it. When I talked with @cescoffier before doing this I believe he mentioned it was only supported when using the rest client.
I will certainly try it though. If it works I can most likely revert all the changes in this class.
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.
@michalszynkiewicz is there info/debug logging in Stork that I can turn on to see whats going on under the covers?
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.
@michalszynkiewicz looks like it does work using the stork://
URL in the WebTarget
. I've changed the commits a bit. I essentially reverted the VillainClient
class and then added in some debug logging.
One question I had though was that when I deploy to k8s I'm using the k8s service discovery. It seems I need to add
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: default_view
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: view
subjects:
- kind: ServiceAccount
name: default
to the namespace in order for the service discovery to work? Is that a true statement?
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 haven't done much work on the kube stuff, @aureamunoz knows best.
Re tracing/logging, that's an area we probably should improve in Stork...
it seems there are some problems with formatting |
What kinds of problems? |
.../src/test/java/io/quarkus/sample/superheroes/fight/HeroesVillainsWiremockServerResource.java
Show resolved
Hide resolved
It seems it's just a one problem after all, I added a comment |
3196a0a
to
26388e5
Compare
Fixes #40
Adding SmallRye Stork service discovery for both the hero & villain services.
The
VillainClient
, is implemented by directly using the JAX-RS client API with the RESTEasy Reactive client. Therefore, the Stork API is used directly in order to get the same functionality available in theHeroRestClient
.Fixes #40