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

Refactor spring retry #331

Merged
merged 9 commits into from Mar 12, 2018

Conversation

@ryanjbaxter
Copy link
Contributor

ryanjbaxter commented Mar 6, 2018

Simplify the number of interfaces involved in configuring retry to one interface.
Removing some deprecated constructors.

@ryanjbaxter ryanjbaxter requested a review from spencergibb Mar 6, 2018
@ryanjbaxter ryanjbaxter added this to the 2.0.0.RC1 milestone Mar 6, 2018
Copy link
Member

spencergibb left a comment

Love all the deletions! I'm so glad you did this. It was getting to be a mess. A couple small things otherwise LGTM.

*/
public BackOffPolicy createBackOffPolicy(String service);

static class DefaultRetryFactory implements LoadBalancedRetryFactory {

This comment has been minimized.

Copy link
@spencergibb

spencergibb Mar 6, 2018

Member

Could we use default methods rather than a static class?

return response;
}, new LoadBalancedRecoveryCallback<ClientHttpResponse, ClientHttpResponse>() {
@Override
protected ClientHttpResponse createResponse(ClientHttpResponse response, URI uri) {

This comment has been minimized.

Copy link
@spencergibb

spencergibb Mar 6, 2018

Member

Looks like this could be pushed up as the default impl of the abstract method too. Simplify even more.

This comment has been minimized.

Copy link
@ryanjbaxter

ryanjbaxter Mar 7, 2018

Author Contributor

I dont think so. This is a special case where both parameters are the same type. In every other case (in Netflix and Feign) the two type parameters are different.

This comment has been minimized.

Copy link
@spencergibb

spencergibb Mar 7, 2018

Member

Maybe a comment then

This comment has been minimized.

Copy link
@ryanjbaxter

ryanjbaxter Mar 7, 2018

Author Contributor

done

@holy12345

This comment has been minimized.

Copy link
Contributor

holy12345 commented Mar 7, 2018

RetryTemplate related information may be placed alone in a method, to make it more concise?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor Author

ryanjbaxter commented Mar 7, 2018

@holy12345 can you be more specific?

@holy12345

This comment has been minimized.

Copy link
Contributor

holy12345 commented Mar 12, 2018

Hi @ryanjbaxter
Sorry, because I was on vacation these days, I did not reply to you.
I want to assemble all the information about RetryTemplate into one method:

private RetryTemplate createRetryTemplate(HttpRequest request, String serviceName, LoadBalancedRetryPolicy retryPolicy ) {
	RetryTemplate template = new RetryTemplate();
	BackOffPolicy backOffPolicy = lbRetryFactory.createBackOffPolicy(serviceName);
	template.setBackOffPolicy(backOffPolicy == null ? new NoBackOffPolicy() : backOffPolicy);
	template.setThrowLastExceptionOnExhausted(true);
	RetryListener[] retryListeners = lbRetryFactory.createRetryListeners(serviceName);
	if (retryListeners != null && retryListeners.length != 0) {
		template.setListeners(retryListeners);
	}
	template.setRetryPolicy(!lbProperties.isEnabled() || retryPolicy == null ? new NeverRetryPolicy()
			: new InterceptorRetryPolicy(request, retryPolicy, loadBalancer, serviceName));
	return template;
}

Because all the above operations are about retryTemplate, so I think we should pull it out into a separate method so that it looks more concise

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor Author

ryanjbaxter commented Mar 12, 2018

@holy12345 Within RetryLoadBalancerInterceptor?

@holy12345

This comment has been minimized.

Copy link
Contributor

holy12345 commented Mar 12, 2018

Yes, what do you think?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor Author

ryanjbaxter commented Mar 12, 2018

Sure makes sense, thanks for the suggestion

@spencergibb spencergibb merged commit 529739d into spring-cloud:master Mar 12, 2018
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
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.