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
Support dynamic properties in HTTP announcement #1157
Conversation
cfede98
to
1a83fe6
Compare
discovery/src/main/java/io/airlift/discovery/client/DiscoveryBinder.java
Outdated
Show resolved
Hide resolved
discovery/src/test/java/io/airlift/discovery/client/TestHttpAnnouncementBinder.java
Show resolved
Hide resolved
discovery/src/main/java/io/airlift/discovery/client/DiscoveryBinder.java
Outdated
Show resolved
Hide resolved
discovery/src/main/java/io/airlift/discovery/client/DiscoveryBinder.java
Outdated
Show resolved
Hide resolved
discovery/src/main/java/io/airlift/discovery/client/DiscoveryBinder.java
Outdated
Show resolved
Hide resolved
discovery/src/main/java/io/airlift/discovery/client/DiscoveryBinder.java
Outdated
Show resolved
Hide resolved
discovery/src/main/java/io/airlift/discovery/client/DiscoveryBinder.java
Show resolved
Hide resolved
1a83fe6
to
84ca213
Compare
AC, PTAL |
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.
The API looks good, but we should still create custom annotation class rather than using Named
. We avoid using Named
as it's single shared namespace. While this usage of using it with UUIDs is unlikely to actually be a problem, using a custom annotation is best practice (and keeps the simple rule of "don't use Named
").
Custom annotation is less encapsulated, as someone could instantiate it outside of the dedicated builder, so i think this usage of Named is actually better. We will need a custom annotation if we want idempotence (#1157 (comment)). Although I don't agree, I will still switch to custom annotation. |
Fix the test to test the class it is supposed to test.
Extend discovery binder's bindHttpAnnouncement's capabilities by allowing bindings of non-constant properties that require Guice injection to be calculated.
84ca213
to
9034c7b
Compare
@@ -0,0 +1,65 @@ | |||
/* |
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.
Drop this header - not required.
Extend discovery binder's bindHttpAnnouncement's capabilities by
allowing bindings of non-constant properties that require Guice
injection to be calculated.
For trinodb/trino#21744