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

Use Spring Retry For Failed Requests Using Feign #1457

Merged
merged 7 commits into from
Jan 10, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,16 @@

package org.springframework.cloud.netflix.feign;

import feign.Contract;
import feign.Feign;
import feign.Logger;
import feign.Retryer;
import feign.codec.Decoder;
import feign.codec.Encoder;
import feign.hystrix.HystrixFeign;

import java.util.ArrayList;
import java.util.List;

import org.springframework.beans.factory.ObjectFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
Expand All @@ -35,16 +42,8 @@
import org.springframework.core.convert.ConversionService;
import org.springframework.format.support.DefaultFormattingConversionService;
import org.springframework.format.support.FormattingConversionService;

import com.netflix.hystrix.HystrixCommand;

import feign.Contract;
import feign.Feign;
import feign.Logger;
import feign.codec.Decoder;
import feign.codec.Encoder;
import feign.hystrix.HystrixFeign;

/**
* @author Dave Syer
* @author Venil Noronha
Expand Down Expand Up @@ -107,7 +106,7 @@ public Feign.Builder feignHystrixBuilder() {
@Scope("prototype")
@ConditionalOnMissingBean
public Feign.Builder feignBuilder() {
return Feign.builder();
return Feign.builder().retryer(Retryer.NEVER_RETRY);
Copy link
Member

Choose a reason for hiding this comment

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

This should go in a @Bean definition.

}

@Bean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@

import java.util.Map;

import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryPolicyFactory;
import org.springframework.cloud.netflix.ribbon.ServerIntrospector;
import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
import org.springframework.retry.support.RetryTemplate;
import org.springframework.util.ConcurrentReferenceHashMap;

import com.netflix.client.config.IClientConfig;
Expand All @@ -34,11 +37,22 @@
public class CachingSpringLoadBalancerFactory {

private final SpringClientFactory factory;
private final RetryTemplate retryTemplate;
private final LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory;

private volatile Map<String, FeignLoadBalancer> cache = new ConcurrentReferenceHashMap<>();

public CachingSpringLoadBalancerFactory(SpringClientFactory factory) {
this.factory = factory;
this.retryTemplate = new RetryTemplate();
this.loadBalancedRetryPolicyFactory = new RibbonLoadBalancedRetryPolicyFactory(factory);
}

public CachingSpringLoadBalancerFactory(SpringClientFactory factory, RetryTemplate retryTemplate,
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the old signature for backwards compatibility.

LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory) {
this.factory = factory;
this.retryTemplate = retryTemplate;
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
}

public FeignLoadBalancer create(String clientName) {
Expand All @@ -48,7 +62,8 @@ public FeignLoadBalancer create(String clientName) {
IClientConfig config = this.factory.getClientConfig(clientName);
ILoadBalancer lb = this.factory.getLoadBalancer(clientName);
ServerIntrospector serverIntrospector = this.factory.getInstance(clientName, ServerIntrospector.class);
FeignLoadBalancer client = new FeignLoadBalancer(lb, config, serverIntrospector);
FeignLoadBalancer client = new FeignLoadBalancer(lb, config, serverIntrospector, retryTemplate,
loadBalancedRetryPolicyFactory);
this.cache.put(clientName, client);
return client;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,54 +16,74 @@

package org.springframework.cloud.netflix.feign.ribbon;

import feign.Client;
import feign.Request;
import feign.Response;
import feign.Util;

import java.io.IOException;
import java.net.URI;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import org.springframework.cloud.client.ServiceInstance;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory;
import org.springframework.cloud.client.loadbalancer.ServiceInstanceChooser;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancerClient;
import org.springframework.cloud.netflix.ribbon.ServerIntrospector;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpRequest;
import org.springframework.retry.RetryCallback;
import org.springframework.retry.RetryContext;
import org.springframework.retry.policy.NeverRetryPolicy;
import org.springframework.retry.support.RetryTemplate;
import com.netflix.client.AbstractLoadBalancerAwareClient;
import com.netflix.client.ClientException;
import com.netflix.client.ClientRequest;
import com.netflix.client.DefaultLoadBalancerRetryHandler;
import com.netflix.client.IResponse;
import com.netflix.client.RequestSpecificRetryHandler;
import com.netflix.client.RetryHandler;
import com.netflix.client.config.CommonClientConfigKey;
import com.netflix.client.config.IClientConfig;
import com.netflix.loadbalancer.ILoadBalancer;
import com.netflix.loadbalancer.Server;

import feign.Client;
import feign.Request;
import feign.Response;
import feign.Util;

import static org.springframework.cloud.netflix.ribbon.RibbonUtils.updateToHttpsIfNeeded;

public class FeignLoadBalancer extends
AbstractLoadBalancerAwareClient<FeignLoadBalancer.RibbonRequest, FeignLoadBalancer.RibbonResponse> {
AbstractLoadBalancerAwareClient<FeignLoadBalancer.RibbonRequest, FeignLoadBalancer.RibbonResponse> implements
ServiceInstanceChooser {

private final int connectTimeout;
private final int readTimeout;
private final IClientConfig clientConfig;
private final ServerIntrospector serverIntrospector;
private final RetryTemplate retryTemplate;
private final LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory;

public FeignLoadBalancer(ILoadBalancer lb, IClientConfig clientConfig,
ServerIntrospector serverIntrospector) {
ServerIntrospector serverIntrospector, RetryTemplate retryTemplate,
LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory) {
super(lb, clientConfig);
this.setRetryHandler(RetryHandler.DEFAULT);
this.retryTemplate = retryTemplate;
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.setRetryHandler(new DefaultLoadBalancerRetryHandler(clientConfig));
this.clientConfig = clientConfig;
this.connectTimeout = clientConfig.get(CommonClientConfigKey.ConnectTimeout);
this.readTimeout = clientConfig.get(CommonClientConfigKey.ReadTimeout);
this.serverIntrospector = serverIntrospector;
}

@Override
public RibbonResponse execute(RibbonRequest request, IClientConfig configOverride)
public RibbonResponse execute(final RibbonRequest request, IClientConfig configOverride)
throws IOException {
Request.Options options;
final Request.Options options;
if (configOverride != null) {
options = new Request.Options(
configOverride.get(CommonClientConfigKey.ConnectTimeout,
Expand All @@ -74,26 +94,34 @@ public RibbonResponse execute(RibbonRequest request, IClientConfig configOverrid
else {
options = new Request.Options(this.connectTimeout, this.readTimeout);
}
Response response = request.client().execute(request.toRequest(), options);
return new RibbonResponse(request.getUri(), response);
LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory.create(this.getClientName(), this);
retryTemplate.setRetryPolicy(retryPolicy == null ? new NeverRetryPolicy()
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should only be done once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the request when creating the policy

Copy link
Member

Choose a reason for hiding this comment

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

I'm still confused about where the request is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you are saying that the policy should only be created once? The problem is we need the Request when FeignRetryPolicy.open is called so we can set the ServiceInstance in the Retry Context.

Copy link
Member

Choose a reason for hiding this comment

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

I see, the retry policy created below uses the request. Could there be a problem since the retry policy is scoped to the request and the template is a field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just trying to make sure I understand the concern....

If multiple requests are executed simultaneously you are concerned about switching out the retry policy mid request?

Copy link
Member

Choose a reason for hiding this comment

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

That's my worry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I guess the only good solution for now is to create a new RetryTemplate for each request. I don't see any real downside to that, but I do think it can be handled better by Spring Retry. I posed this in issue spring-projects/spring-retry#71.

: new FeignRetryPolicy(request.toHttpRequest(), retryPolicy, this, this.getClientName()));
return retryTemplate.execute(new RetryCallback<RibbonResponse, IOException>() {
@Override
public RibbonResponse doWithRetry(RetryContext retryContext) throws IOException {
Request feignRequest = null;
//on retries the policy will choose the server and set it in the context
//extract the server and update the request being made
if(retryContext instanceof LoadBalancedRetryContext) {
ServiceInstance service = ((LoadBalancedRetryContext)retryContext).getServiceInstance();
if(service != null) {
feignRequest = ((RibbonRequest)request.replaceUri(reconstructURIWithServer(new Server(service.getHost(), service.getPort()), request.getUri()))).toRequest();
}
}
if(feignRequest == null) {
feignRequest = request.toRequest();
}
Response response = request.client().execute(feignRequest, options);
return new RibbonResponse(request.getUri(), response);
}
});
}

@Override
public RequestSpecificRetryHandler getRequestSpecificRetryHandler(
RibbonRequest request, IClientConfig requestConfig) {
if (this.clientConfig.get(CommonClientConfigKey.OkToRetryOnAllOperations,
false)) {
return new RequestSpecificRetryHandler(true, true, this.getRetryHandler(),
requestConfig);
}
if (!request.toRequest().method().equals("GET")) {
return new RequestSpecificRetryHandler(true, false, this.getRetryHandler(),
requestConfig);
}
else {
return new RequestSpecificRetryHandler(true, true, this.getRetryHandler(),
requestConfig);
}
return new RequestSpecificRetryHandler(false, false, this.getRetryHandler(), requestConfig);
}

@Override
Expand All @@ -102,6 +130,12 @@ public URI reconstructURIWithServer(Server server, URI original) {
return super.reconstructURIWithServer(server, uri);
}

@Override
public ServiceInstance choose(String serviceId) {
return new RibbonLoadBalancerClient.RibbonServer(serviceId,
this.getLoadBalancer().chooseServer(serviceId));
}

static class RibbonRequest extends ClientRequest implements Cloneable {

private final Request request;
Expand Down Expand Up @@ -129,6 +163,33 @@ Client client() {
return this.client;
}

HttpRequest toHttpRequest() {
return new HttpRequest() {
@Override
public HttpMethod getMethod() {
return HttpMethod.resolve(RibbonRequest.this.toRequest().method());
}

@Override
public URI getURI() {
return RibbonRequest.this.getUri();
}

@Override
public HttpHeaders getHeaders() {
Map<String, List<String>> headers = new HashMap<String, List<String>>();
Map<String, Collection<String>> feignHeaders = RibbonRequest.this.toRequest().headers();
for(String key : feignHeaders.keySet()) {
headers.put(key, new ArrayList<String>(feignHeaders.get(key)));
}
HttpHeaders httpHeaders = new HttpHeaders();
httpHeaders.putAll(headers);
return httpHeaders;

}
};
}

@Override
public Object clone() {
return new RibbonRequest(this.client, this.request, getUri());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
*
* * Copyright 2013-2016 the original author or authors.
* *
* * Licensed under the Apache License, Version 2.0 (the "License");
* * you may not use this file except in compliance with the License.
* * You may obtain a copy of the License at
* *
* * http://www.apache.org/licenses/LICENSE-2.0
* *
* * Unless required by applicable law or agreed to in writing, software
* * distributed under the License is distributed on an "AS IS" BASIS,
* * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* * See the License for the specific language governing permissions and
* * limitations under the License.
*
*/

package org.springframework.cloud.netflix.feign.ribbon;

import java.net.URI;
import java.util.HashMap;
import java.util.Map;

import org.springframework.cloud.client.ServiceInstance;
import org.springframework.cloud.client.loadbalancer.InterceptorRetryPolicy;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy;
import org.springframework.cloud.client.loadbalancer.ServiceInstanceChooser;
import org.springframework.http.HttpRequest;
import org.springframework.retry.RetryContext;

/**
* @author Ryan Baxter
*/
public class FeignRetryPolicy extends InterceptorRetryPolicy {
private HttpRequest request;
private String serviceId;
public FeignRetryPolicy(HttpRequest request, LoadBalancedRetryPolicy policy, ServiceInstanceChooser serviceInstanceChooser, String serviceName) {
super(request, policy, serviceInstanceChooser, serviceName);
this.request = request;
this.serviceId = serviceName;
}

@Override
public boolean canRetry(RetryContext context) {
/*
* In InterceptorRetryPolicy.canRetry we ask the LoadBalancer to choose a server if one is not
* set in the retry context and then return true. RetryTemplat calls the canRetry method of
* the policy even on its first execution. So the fact that we didnt have a service instance set
* in the RetryContext signaled that it was the first execution and we should return true.
*
* In the Feign scenario, Feign as actually already queried the load balancer for a service instance
* and we set that service instance in the context when we call the open method of the policy. So in
* the Feign case we just return true if the retry count is 0 indicating we haven't yet made a failed
* request.
*/
if(context.getRetryCount() == 0) {
return true;
}
return super.canRetry(context);
}

@Override
public RetryContext open(RetryContext parent) {
/*
* With Feign (unlike Ribbon) the request already has the URI for the service instance
* we are going to make the request to, so extract that information and set the service
* instance in the context. In the Ribbon scenario the URI in the request object still has
* the service id so we choose and set the service instance later on.
*/
LoadBalancedRetryContext context = new LoadBalancedRetryContext(parent, this.request);
context.setServiceInstance(new FeignRetryPolicyServiceInstance(serviceId, request));
return context;
}

class FeignRetryPolicyServiceInstance implements ServiceInstance {

private String serviceId;
private HttpRequest request;
private Map<String, String> metadata;

FeignRetryPolicyServiceInstance(String serviceId, HttpRequest request) {
this.serviceId = serviceId;
this.request = request;
this.metadata = new HashMap<String, String>();
}

@Override
public String getServiceId() {
return serviceId;
}

@Override
public String getHost() {
return request.getURI().getHost();
}

@Override
public int getPort() {
return request.getURI().getPort();
}

@Override
public boolean isSecure() {
return "https".equals(request.getURI().getScheme());
}

@Override
public URI getUri() {
return request.getURI();
}

@Override
public Map<String, String> getMetadata() {
return metadata;
}
}
}
Loading