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

SystemSocket support - alternative approach #235

Merged
merged 7 commits into from
Oct 31, 2014
Merged

SystemSocket support - alternative approach #235

merged 7 commits into from
Oct 31, 2014

Conversation

keithbranton
Copy link

Alternative pull request including refactor to DiscoveryClient so that it uses a factory method.

keithbranton and others added 6 commits October 28, 2014 10:00
@cloudbees-pull-request-builder

eureka-pull-requests #293 SUCCESS
This pull request looks good

@@ -145,6 +232,18 @@ public CustomApacheHttpClientConfig(String clientName, int maxConnectionsPerHost
}
}

private static class ProxyCustomApacheHttpClientConfig extends DefaultApacheHttpClient4Config {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is never used.

@tbak
Copy link

tbak commented Oct 30, 2014

The implementation looks good, and covers all the important cases that I can think of.
@NiteshKant Can you have a look at this pull request?

configuration in EurekaJerseyClient.createProxyJerseyClient when I
initially refactored this.
@cloudbees-pull-request-builder

eureka-pull-requests #294 FAILURE
Looks like there's a problem with this pull request

if (eurekaServiceUrls.get().get(0).startsWith("https://") &&
"true".equals(System.getProperty("com.netflix.eureka.shouldSSLConnectionsUseSystemSocketFactory"))) {
discoveryJerseyClient = EurekaJerseyClient.createSystemSSLJerseyClient("DiscoveryClient-HTTPClient-System",
clientConfig.getEurekaServerConnectTimeoutSeconds() * 1000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just pass the client config to the EurekaJerseyClient?

@NiteshKant
Copy link
Contributor

The change looks good to me.

tbak pushed a commit that referenced this pull request Oct 31, 2014
SystemSocket support - alternative approach
@tbak tbak merged commit 3c5424d into Netflix:master Oct 31, 2014
@tbak
Copy link

tbak commented Oct 31, 2014

@keithbranton Thank you for your contribution.

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

Successfully merging this pull request may close these issues.

None yet

4 participants