-
Notifications
You must be signed in to change notification settings - Fork 579
Introduces EurekaRegistrationFeature #9789
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
Conversation
...ions/eureka/eureka/src/test/java/io/helidon/integrations/eureka/TestAgainstEurekaServer.java
Outdated
Show resolved
Hide resolved
...ns/eureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationFeature.java
Outdated
Show resolved
Hide resolved
...ns/eureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationFeature.java
Outdated
Show resolved
Hide resolved
...ns/eureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationFeature.java
Outdated
Show resolved
Hide resolved
...ns/eureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationFeature.java
Outdated
Show resolved
Hide resolved
...ns/eureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationFeature.java
Outdated
Show resolved
Hide resolved
...ns/eureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationFeature.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Laird Nelson <[email protected]>
…ound Status equality issues Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
da8beab
to
7f060ff
Compare
…Adds requested integration test and documentation Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
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.
A few comments/questions/corrections.
docs/src/main/asciidoc/se/integrations/eureka/eureka-registration.adoc
Outdated
Show resolved
Hide resolved
implementations. This feature is one such implementation. | ||
<2> Information about the HTTP client the feature uses to communicate with Eureka is a child of this node. | ||
<3> The `base-uri` needs to identify an available Netflix Eureka Server of at least version | ||
{version-lib-eureka}. Netflix Eureka Server is commonly made available on port `8761`. |
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.
Does our code default the port to 8761 if the user does not specify it? Should we?
Maybe the doc should say either way.
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.
My thinking here is that the Eureka server is out of our control, so we should leave defaults alone. What say you?
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.
Ah, so if we don't set the value in the Eureka API then Eureka applies its defaults.
Maybe the doc can say just that?
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.
Well, there aren't any defaults to apply here. You have to set the base-uri
using the HttpClientConfig
API (via configuration). If you don't, the integration doesn't know where the Eureka server is. I spent a while reverse engineering some things on the Eureka side to figure out what this URI should look like and wanted to save someone else the pain. There's no requirement that the Eureka server expose itself on port 8761, or that its context path be /eureka
, but those do seem to be common values encountered in the wild. Open to suggestions on how to convey all this.
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 was thinking from a user's perspective, not necessarily from how the Helidon code decides to implement the configuration (which, don't get me wrong, as written, makes perfect sense). For a generic HTTP server there's no requirement that it listen on port 80, but that's common so users might expect to be able to omit that from configuration specifying a URI to a generic webserver.
In the context of our Eureka integration, I was imagining that the user is thinking about specifying how to connect to a Eureka server, and if port 8761 is common (though not required) it seems to me that such a user would be thinking "the Helidon integration will connect using port 8761 if I don't specify otherwise."
My doc point was that, if our code is not providing the common port as a default then the doc maybe could say "The base-uri
needs to identify the full URI--including host, port, and path-- to an available Netflix Eureka Server" (italics just for emphasis in this comment and there might be better phrases to convey this). That's implied by the next sentence in the doc which says this client config is wholly defined by HttpClientConfig
, implied so long as a reader reads that and thinks "oh, yes, that's for a connecting to a generic webserver which would use a default port of 80 but I don't want 80 so I'd better specify 8761."
I guess I do not expect readers thinking about setting up config for connecting to a Eureka server to have the above realization on their own, hence my search for some way to emphasize this.
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.
a user would be thinking "the Helidon integration will connect using port 8761 if I don't specify otherwise."
Right, but unless I'm missing something that would involve my checking the Http1ClientBuilder
's baseUri
property, cracking it open to see what the port is, and then filling in the details for what was, in all other respects, a fully-specified base URI value. Was it truly fully specified? Or not? How could I tell? I think this is a limitation (one of many) in the configuration mechanism, but if the baseUri
property is treated as atomic, I think I have to respect 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 originally thought that it might be simpler than it turns out for Helidon to apply Eureka-endpoint defaults to omitted portions of the URI but I have seen the light that this would be more involved than is worth it.
That's why my latest comment shifted to suggesting the doc emphasize that the user should not assume that Helidon will supply Eureka-inspired defaults to omitted parts of the URI. But I tried to phrase it in a positive way ("specify the entire URI") instead of a negative way ("don't expect us to default the port or path for you").
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.
Got it; so either as part of this PR (if there's time, not sure) or a follow-on I should change the base URI documentation to indicate that it should fully identify a Eureka server (scheme, host, port, context path), and perhaps offer a quick example.
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.
How about if (now or later) I change the relevant part of that .adoc
file to read:
<3> The
base-uri
needs to identify an available Netflix Eureka Server of at least version {version-lib-eureka}. The URI must be absolute, and must specify a scheme (oftenhttps
), a host, a port (often8761
), and a path identifying the root of a Netflix Eureka Server installation (often/eureka
).
…?
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.
Superb!
...ureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationHttpFeature.java
Show resolved
Hide resolved
...ureka/eureka/src/main/java/io/helidon/integrations/eureka/EurekaRegistrationHttpFeature.java
Show resolved
Hide resolved
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
Signed-off-by: Laird Nelson <[email protected]>
This PR introduces
EurekaRegistrationFeature
to implement the goals described in #9704.Documentation will follow once the PR stabilizes.