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

Provide The Ability to Exclude Global RetryListeners #211

Closed
krooj opened this issue Jul 17, 2020 · 7 comments
Closed

Provide The Ability to Exclude Global RetryListeners #211

krooj opened this issue Jul 17, 2020 · 7 comments
Milestone

Comments

@krooj
Copy link

krooj commented Jul 17, 2020

Hey,

I recently encountered this case whereby methods which are annotated @Retryable with no list of listeners being passed automatically has them added to the executing RetryTemplate at runtime. Upon reading up on the source, this is expected, since RetryListener instances which are injected into the application context become, so called, "global" listeners. Unfortunately, this has unexpected consequences in some cases, and it would be awesome to also be able to specify a list of excludeListeners in the @Retryable annotation.

I know it's feasible to also pass a label and introspect it at the listener level to make the execution decision, but that, afaik, isn't the intended purpose of that parameter.

@garyrussell
Copy link
Contributor

A simple work around would be to add

@Bean
RetryListener noOpListener() {
    return new RetryListenerSupport();
}

and listeners = "noOpListener".

@krooj
Copy link
Author

krooj commented Jul 17, 2020

A simple work around would be to add

@Bean
RetryListener noOpListener() {
    return new RetryListenerSupport();
}

and listeners = "noOpListener".

It's a little funky and will require documentation to the effect... I'll give it a go, but my initial reaction was to migrate to declarative code.

@ssjf409
Copy link

ssjf409 commented Oct 13, 2023

May I handle this issue? please assign to me~ I'll create PR soon!

@artembilan
Copy link
Member

Well, this is what is said on that option:

	/**
	 * Bean names of retry listeners to use instead of default ones defined in Spring
	 * context
	 * @return retry listeners bean names
	 */
	String[] listeners() default {};

I suggest to make ignoring global listeners is to modify the logic to react to something like this: listeners = "".
Of course, with the meaning "no listeners at all".

Right now it fails like:

org.springframework.beans.factory.NoSuchBeanDefinitionException: No bean named '' available

So, that AnnotationAwareRetryOperationsInterceptor.getListenersBeans(String[] listenersBeanNames) has to be modified to react to the empty string.
This is going to be a new behavior without breaking change an existing one where we don't have a listeners, therefore rely on global configuration.

@ssjf409 ,

Feel free to provide a contribution, but we assign issues only to team members.

Thanks

akenra pushed a commit to akenra/spring-retry that referenced this issue Oct 21, 2023
akenra pushed a commit to akenra/spring-retry that referenced this issue Oct 22, 2023
akenra pushed a commit to akenra/spring-retry that referenced this issue Oct 22, 2023
akenra pushed a commit to akenra/spring-retry that referenced this issue Oct 22, 2023
akenra pushed a commit to akenra/spring-retry that referenced this issue Oct 22, 2023
ssjf409 added a commit to ssjf409/spring-retry that referenced this issue Oct 22, 2023
@ssjf409
Copy link

ssjf409 commented Oct 22, 2023

Hi. akenra. I saw your PR.
It was nice PR.

Before I created PR, I thought two options

I came up with two different implementations of @artembilan's suggestion.
The first is literally when the listeners attribute is set to empty string, it is the same as if no listener is set.
The second is that when a non-existent listener is set, it is the same as if no listener is set.
I appreciate your nice implementation of the first method. It was also a study for me.

However, from the user's perspective, it felt strange to have to know how it behaves when the listeners is set to an empty string.
I thought it was natural that when the listeners attribute is set to a non-existent listener, it would behave as if it were set to a listener without any configurations.

So I create this PR.

@artembilan
Copy link
Member

I wish there was no a global interceptors usage over there at all.
It is much better to be as clear as possible. And then the config would be just straightforward: no listeners attribute - no listeners inserted. Provided listeners - use only those.
But since the cat is out of he bag for those global listeners, we might come up with some solution working for everyone.
The empty value for listeners is clear enough for me that we don't configure anything. It simply can be documented in JavaDocs for that attribute.
Another solution, where it might be more precise is to document to use AnnotationConstants.NULL.

The non-existent listener approach is vague and may cause confusion and maybe resolved to something what is not expected in the target project.

@ssjf409
Copy link

ssjf409 commented Oct 23, 2023

@artembilan, Thanks for the clear guide.
I think akenra's PR already cover the your suggestion.
I will close my PR(#393)

@artembilan artembilan added this to the 2.0.5 milestone Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants