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

NettyHttpClient created from RibbonTransport does not honor NIWSServerListClassName in IClientConfig #136

Closed
allenxwang opened this issue Jul 16, 2014 · 8 comments · Fixed by #137
Labels
Milestone

Comments

@allenxwang
Copy link

The constructor calls the LoadBalancerBuilder API which always return a load balancer with a ConfigurationBasedServerList, regardless whether NIWSServerListClassName is set in the config.

@allenxwang allenxwang added the bug label Jul 16, 2014
@allenxwang allenxwang added this to the 2.0-RC4 milestone Jul 16, 2014
allenxwang pushed a commit to allenxwang/ribbon that referenced this issue Jul 16, 2014
@allenxwang allenxwang mentioned this issue Jul 16, 2014
@aspyker
Copy link
Contributor

aspyker commented Jul 16, 2014

I tried this with my project. I still see similar behavior. What specific configuration is needed in the properties?

@allenxwang
Copy link
Author

@aspyker The properties required are the same as those for RestClient:

DeploymentContextBasedVipAddresses
NIWSServerListClassName

Here is an example showing Ribbon 2 working with a mocked DiscoveryClient:

https://github.com/allenxwang/ribbon/blob/cp/ribbon/src/test/java/com/netflix/ribbon/DiscoveryEnabledServerListTest.java

allenxwang pushed a commit that referenced this issue Jul 17, 2014
@aspyker
Copy link
Contributor

aspyker commented Jul 17, 2014

@allenxwang I think your test is only working as you mock out the DiscoveryClient. I changed yours to instead do the following:

@Override
protected String getVipAddress() {
    return "ACMEAIR_AUTH_SERVICE";
}

@Test
public void testDynamicServers() {
    ConfigurationManager.getConfigInstance().setProperty("MyService.ribbon." + Keys.DeploymentContextBasedVipAddresses, getVipAddress());
    ConfigurationManager.getConfigInstance().setProperty("MyService.ribbon." + Keys.NIWSServerListClassName, DiscoveryEnabledNIWSServerList.class.getName());
    ConfigurationManager.getConfigInstance().setProperty("eureka.shouldUseDns", "false");
    ConfigurationManager.getConfigInstance().setProperty("eureka.serviceUrl.default", "http://172.17.0.15/eureka/v2/");

and in your mock:

public void setupMock(){

// List instances = getDummyInstanceInfo("dummy", getMockServerList());
// PowerMock.mockStatic(DiscoveryManager.class);
// PowerMock.mockStatic(DiscoveryClient.class);
//
// DiscoveryClient mockedDiscoveryClient = createMock(DiscoveryClient.class);
// DiscoveryManager mockedDiscoveryManager = createMock(DiscoveryManager.class);
//
// expect(DiscoveryClient.getZone((InstanceInfo) EasyMock.anyObject())).andReturn("dummyZone").anyTimes();
// expect(DiscoveryManager.getInstance()).andReturn(mockedDiscoveryManager).anyTimes();
// expect(mockedDiscoveryManager.getDiscoveryClient()).andReturn(mockedDiscoveryClient).anyTimes();
//
// expect(mockedDiscoveryClient.getInstancesByVipAddress(getVipAddress(), false, null)).andReturn(instances).anyTimes();
//
// replay(DiscoveryManager.class);
// replay(DiscoveryClient.class);
// replay(mockedDiscoveryManager);
// replay(mockedDiscoveryClient);
}

This is a valid eureka server as well as vipAddress as:

ispyker:~ aspyker$ curl http://172.17.0.15/eureka/v2/apps/ACMEAIR_AUTH_SERVICE

ACMEAIR_AUTH_SERVICE

ispyker.raleigh.ibm.com
ACMEAIR_AUTH_SERVICE
9.27.117.176
UP
...

The problem in tracing my test code and yours with this change to not have a mocked eureka is:

com.netflix.niws.loadbalancer.DiscoveryEnabledNIWSServerList:

private List<DiscoveryEnabledServer> obtainServersViaDiscovery() {
    List<DiscoveryEnabledServer> serverList = new ArrayList<DiscoveryEnabledServer>();

    DiscoveryClient discoveryClient = DiscoveryManager.getInstance()
            .getDiscoveryClient();
    if (discoveryClient == null) {
        return new ArrayList<DiscoveryEnabledServer>();
    }

With your move, the getDiscoveryClient is coming back as non-null due to your mocking. When eureka isn't mocked, it comes back as null.

Can you confirm that this code works when you don't mock Eureka?

@aspyker
Copy link
Contributor

aspyker commented Jul 17, 2014

@allenxwang formattting issues with code. The curl command comes back with XML that shows the instance as up.

@allenxwang
Copy link
Author

@aspyker If you run the test without mocking, DiscoveryManager.getInstance().getDiscoveryClient() will return null. If you run the same code in Karyon, DiscoveryManager.getInstance().getDiscoveryClient() should not return null as Karyon should properly initialize DiscoveryClient. If you observe DiscoveryManager.getInstance().getDiscoveryClient() returns null in Karyon, please fire a bug against Karyon.

@aspyker
Copy link
Contributor

aspyker commented Jul 17, 2014

Ok that was my worry as most of my work before was in Karyon. Do you know of a way to get Eureka property configured in a non-Karyon environment - or is this a question to ask over in the Eureka projects. Will move to Karyon immediately instead of J2SE and see what happens.

@allenxwang
Copy link
Author

@aspyker I think there is a way to manually initialize DiscoveryClient. This is the code from DiscoveryManager:

    public void initComponent(EurekaInstanceConfig config,
            EurekaClientConfig eurekaConfig) {
        this.eurekaInstanceConfig = config;
        this.eurekaClientConfig = eurekaConfig;
        if (ApplicationInfoManager.getInstance().getInfo() == null) {
            // Initialize application info
            ApplicationInfoManager.getInstance().initComponent(config);
        }
        InstanceInfo info = ApplicationInfoManager.getInstance().getInfo();
        discoveryClient = new DiscoveryClient(info, eurekaConfig);
    }

So you would need to do something like this:

    DiscoveryManager.getInstance().init(config, eurekaConfig);

You should be able to get further details about how to obtain EurekaInstanceConfig and EurekaClientConfig from Eureka project.

@aspyker
Copy link
Contributor

aspyker commented Jul 21, 2014

Today I was able to move my client to be a Karyon hosted service which meant that Eureka registration/initialization happened. After doing this, I do see (with your ribbon cp branch on github) it paying attention to the archiaus properties and a eureka located service worked fine with:

acmeair-auth-service-client.ribbon.NIWSServerListClassName=com.netflix.niws.loadbalancer.DiscoveryEnabledNIWSServerList
acmeair-auth-service-client.ribbon.DeploymentContextBasedVipAddresses=ACMEAIR_AUTH_SERVICE

It would nice to have a simple Eureka+Ribbon J2SE sample that showed how to initialize Eureka, but for my needs, Karyon server is fine.

I think when I opened the original issue, the problem was with my sample app (not using Karyon and/or not initializing Eureka in J2SE). I'm glad you found an issue with Archaius in your investigation, but likely I can't confirm that issue as it al works for me now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants