Skip to content
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 jersey2 for client/server #821

Merged
merged 3 commits into from Aug 23, 2016

Conversation

@mattnelson
Copy link
Contributor

mattnelson commented Jul 20, 2016

Initial support for jersey2 for #600

Deployed these changes to our local snapshot repository. I've been running our builds with the snapshot for the last week. I believe the only non-passive change for existing jersey1 consumers is the relocation of DiscoveryClientOptionalArgs from an inner class of DiscoveryClient this was because the optional args has a method to supply additional filters which the interface was changed with jaxrs2.

client

Changed DiscoveryClient to accept a TransportClientFactories on the optional args class to support supplying the jersey2 transport client factory.

Jersey2DiscoveryClientOptionalArgs args = new Jersey2DiscoveryClientOptionalArgs();
args.setTransportClientFactories(Jersey2TransportClientFactories.getInstance());
DiscoveryManager.getInstance().initComponent(new MyDataCenterInstanceConfig(), new DefaultEurekaClientConfig(), args);

server

Added EurekaServerContextBinder to use the hk2 injectors for EurekaServerContext. The GuiceFilter is no longer required to be on the filter chain. I did not create a eureka-server-jersey2 module since I am not deploying eureka-server as a war. Added Jersey2EurekaBootStrap which uses the client/server jersey2 classes for peer replication.

// Expects that you have already initialized the discovery client
new Jersey2EurekaBootStrap(DiscoveryManager.getInstance().getDiscoveryClient()
@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Jul 21, 2016

@mattnelson thank you for the PR! Much appreciated. I'll take a look at this as soon as I can.

@@ -82,6 +81,12 @@

protected volatile EurekaServerContext serverContext;
protected volatile AwsBinder awsBinder;

private DiscoveryClient discoveryClient;

This comment has been minimized.

Copy link
@qiangdavidliu

qiangdavidliu Jul 22, 2016

Contributor

I think it is reasonable to add getEurekaClientConfig() to the EurekaClient interface, which will then allow the reference here to be to the EurekaClient interface instead of the DiscoveryClient implementation.


private DiscoveryClient discoveryClient;

protected void setDiscoveryClient(DiscoveryClient discoveryClient) {

This comment has been minimized.

Copy link
@qiangdavidliu

qiangdavidliu Jul 22, 2016

Contributor

Rather than providing overrideable ways to set the client, maybe we can add the necessary constructors to EurekaBootstrap:

public EurekaBootstrap(ApplicationInfoManager infoManager, EurekaClient client) {
    this... = ...
}

public EurekaBootstrap() {
    this(null, null);
}

clientConfig,
additionalFilters,
myInstanceInfo,
new EurekaClientIdentity(myInstanceInfo.getIPAddr())

This comment has been minimized.

Copy link
@qiangdavidliu

qiangdavidliu Jul 22, 2016

Contributor

It might be worthwhile creating a jersey2 version of the EurekaClientIdentity here such that they identify as jersey2 variants of the eureka-client.

@Override
public void destroyResources() {
if (eurekaConnCleaner != null) {
eurekaConnCleaner.shutdown();

This comment has been minimized.

Copy link
@qiangdavidliu

qiangdavidliu Jul 22, 2016

Contributor

In the existing code the connCleaner is called one last time before it is shutdown. Do you want to replicate that here too?

: args.additionalFilters;

EurekaJerseyClient providedJerseyClient = args == null
? null
: args.eurekaJerseyClient;


TransportClientFactories transportClientFactories = TransportClientFactories.INSTANCE;

This comment has been minimized.

Copy link
@qiangdavidliu

qiangdavidliu Jul 22, 2016

Contributor

Can we try another way to do this here instead of having a default static instance reference of a child class in the TransportClientFactories interface? One suggestion could be:

  • define the interface for TransportClientFactories as is, but with no default static instance
  • have a TransportClientFactoriesProvider, e.g.
public class TransportClientFactoriesProvider implements Provider<TransportClientFactories<?>> {

    private volatile TransportClientFactories<?> transportClientFactories;

    public TransportClientFactoriesProvider(TransportClientFactories<?> transportClientFactories) {
        this.transportClientFactories = transportClientFactories;
    }

    @Override
    public synchronized TransportClientFactories get() {
        if (transportClientFactories == null) {
            transportClientFactories = new Jersey1TransportClientFactories();
        }

        return transportClientFactories;
    }
}
  • in DiscoveryClient, initialize this provider with args.getTransportClientFactories(), then just get the actual impl out here in the same way.
@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Jul 22, 2016

@mattnelson are you running both jersey2 client and server in your set up, or just the client?

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Jul 22, 2016

@spencergibb you'll be interested in this PR too.

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Jul 22, 2016

@qiangdavidliu

are you running both jersey2 client and server in your set up, or just the client?

I am running both client and server. The environment I'm using has 4 clients and 1 server (I'll work on getting another server integrated to verify cluster replication) that are able to register and use ribbon with DiscoveryEnabledNIWSServerList.

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Jul 25, 2016

@mattnelson much appreciated. It would be good to have some confirmation that replication is also unaffected once you have >1 server running.

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Jul 26, 2016

Addressed comments on 7dbea2d

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Aug 4, 2016

@qiangdavidliu Finally had a chance to validate replication.

Had 36 clients register with a cluster of 3 eureka servers. Then had an additional 2 eureka servers that were only configured with the other 3 eureka servers to ensure that the apps they got were only through replication. Took a while because I had to trace down a bug in PeerEurekaNodes. isThisMyUrl[1] and EurekaTransportConfig.applicationsResolverUseIp[2]. The virtualization environment I was using only supports IP resolution. I'll get a PR out for those changes as it will be a blocker for me to validate a release candidate of these changes.

[1] https://github.com/Netflix/eureka/blob/v1.4.10/eureka-core/src/main/java/com/netflix/eureka/cluster/PeerEurekaNodes.java#L230-L234
[2] https://github.com/Netflix/eureka/blob/v1.4.10/eureka-client/src/main/java/com/netflix/discovery/shared/transport/EurekaTransportConfig.java#L31

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Aug 11, 2016

@mattnelson great to hear, thanks!

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Aug 17, 2016

@qiangdavidliu Is there anything else you need in order to get this PR merged?

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Aug 17, 2016

@mattnelson nothing major, I just thought (per your last message) that you were making some additional changes to the PR for replication?

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Aug 17, 2016

I can include them with this PR if you want, but they are not related to the jersey2 uplift. They are isolated to running eureka in IP address resolver transport mode.

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Aug 17, 2016

Ah. In that case then all good. A separate PR would be better if they are unrelated. Let me make a release of eureka without this going in first to have an artifact for the current head before merging this in.

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Aug 17, 2016

Created #831 for the IP address transport issue.

@mattnelson

This comment has been minimized.

Copy link
Contributor Author

mattnelson commented Aug 22, 2016

@qiangdavidliu Now that https://github.com/Netflix/eureka/releases/tag/v1.4.11 is released, are there any other blockers?

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Aug 22, 2016

@mattnelson I'm looking at merging this in and doing a release. There is one last issue where due to the move of "DiscoveryClientOptionalArgs" it creates a minor api incompatibility. I am assessing the blast radius of this at the moment.

@qiangdavidliu

This comment has been minimized.

Copy link
Contributor

qiangdavidliu commented Aug 23, 2016

@mattnelson there are a few small changes that I would like to make to this PR, but in the interest of not blocking you further, what do you say to moving this PR to against the (newly created) "contrib/jersey2" branch, where we can merge it in, and then I will make some PRs for the minor changes before we moving it back into master?

edit: I'm actually able to change the base branch, so have done the above.

@qiangdavidliu qiangdavidliu changed the base branch from master to contrib/jersey2 Aug 23, 2016
@qiangdavidliu qiangdavidliu merged commit 620a6c2 into Netflix:contrib/jersey2 Aug 23, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@srinivas-nagesh

This comment has been minimized.

Copy link

srinivas-nagesh commented May 18, 2018

@mattnelson Can you please share any documentation available to use Eureka with Jersey2. I am looking to run Eureka Server as a packaged web app in a servlet container which isn't compatible with Jersey 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.