-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fixme 2.0 patch1 #1473
Fixme 2.0 patch1 #1473
Conversation
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class Jersey3RemoteRegionClientFactory implements TransportClientFactory { |
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.
This implementation came from the now deleted EurekaServerHttpClients
https://github.com/Netflix/eureka/pull/1473/files#diff-bcd6df8c6a0b1c6ebf2d76ccb55219b364009609e9b57df874f9401f31faa2be
...y3/src/main/java/com/netflix/discovery/shared/transport/jersey3/EurekaJersey3ClientImpl.java
Show resolved
Hide resolved
|
||
public DiscoveryClient(ApplicationInfoManager applicationInfoManager, EurekaClientConfig config) { | ||
this(applicationInfoManager, config, null); | ||
@SuppressWarnings("rawtypes") |
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.
Now TransportClientFactories is mandatory. This would break anyone who doesn't use SB cloud. Is there a way that we can construct a default TransportClientFactories for users and provide ctor option w/o TransportClientFactories? I know it's a major release, but I'm not sure how users would know about what to do to construct a TransportClientFactories?
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.
Not without merging jersey 3 modules into core and client. Spring cloud netflix will auto-configure the right thing. Right now there is no guice support, so until that happens no one can use it without spring cloud netflix anyway. If you do get guice support, then that would do the right thing. No one should be trying to construct a discovery client by hand.
EurekaServerIdentity identity = new EurekaServerIdentity(ip);*/ | ||
// FIXME 2.0 | ||
// discoveryApacheClient.addFilter(new EurekaIdentityHeaderFilter(identity)); | ||
EurekaServerIdentity identity = new EurekaServerIdentity(ip); |
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.
Should these lines still be here? There's no more FIXME 2.0
so I'm not sure.
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.
yeah, those are for the non-experimental client I still need to add back. If you look at the top of the comment block, there is a FIXME: 2.0
still
This is basically EurekaServerHttpClients, but as an interface. Various constructors changed from taking EurekaHttpClient to EurekaServerHttpClientFactory
Adds EurekaBootStrap.getDiscoveryClientOptionalArgs() protected method.
Removes deprecated DiscoveryClient constructors.
904e470
to
3b73bdb
Compare
Creates new EurekaServerHttpClientFactory
This is basically EurekaServerHttpClients, but as an interface. Various constructors changed from taking EurekaHttpClient to EurekaServerHttpClientFactory.
Adds TransportClientFactories as required parameter to DiscoveryClient.
Removes deprecated DiscoveryClient constructors.