Expose the MethodInvocation in the MethodInvocationRetryCallback #172
Conversation
…rder to permit to concrete RetryListener implementations to inspect in detail the method called as well as its arguments. This information can be used in the retry listener for either detailed logging or monitoring for the proxied method invocations.
pom.xml
Outdated
| @@ -218,6 +218,12 @@ | |||
| <version>4.12</version> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.hamcrest</groupId> | |||
| <artifactId>hamcrest-all</artifactId> | |||
There was a problem hiding this comment.
Isn't there a way to avoid one more dependency?
I'm not a fun to introduce a dependency just for one test-case.
We would need to manage its dependency in the future and handle code for compatibility.
There was a problem hiding this comment.
Sure.
I've dropped the hamcrest-all dependency and used vanilla junit 4 assertions.
BTW @artembilan would it make sense to have a conversion of the tests of the project to junit 5 ? I'd be willing to do this.
There was a problem hiding this comment.
This project is still aimed to support Java 6 and that's why it's Spring base like is 4.x, too.
I'm not sure that it is going to be so easy to move to JUnit 5 and still support Spring 4 and Java 6...
There was a problem hiding this comment.
According to https://junit.org/junit5/docs/current/user-guide/#overview-java-versions it needs at least java 8.
In the README.md of the project is not mentioned though a reference about the supported java/spring versions. I guess it would make sense to have this.
There was a problem hiding this comment.
There was a problem hiding this comment.
There is a ListenableFuture abstraction in Spring for that reason.
Although it is fully different story and not related to what you are doing here...
There was a problem hiding this comment.
@ashamukov thanks for pointing out the existence of async-retry project. I didn't know about it before.
src/main/java/org/springframework/retry/interceptor/MethodInvocationRetryCallback.java
Show resolved
Hide resolved
src/main/java/org/springframework/retry/interceptor/MethodInvocationRetryCallback.java
Show resolved
Hide resolved
src/main/java/org/springframework/retry/listener/MethodInvocationRetryListenerSupport.java
Outdated
Show resolved
Hide resolved
src/main/java/org/springframework/retry/listener/MethodInvocationRetryListenerSupport.java
Show resolved
Hide resolved
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; |
There was a problem hiding this comment.
I think there was the reason to keep static imports in the end of the list.
I even believe that Checkstyle is going to fail with your change...
There was a problem hiding this comment.
I've came across mvn spring-javaformat:apply and this helped me in doing the reformatting.
When doing the checkstyle checks on the other hand (via mvn validate and the checkstyle config taken from https://github.com/spring-io/spring-javaformat the build fails with quite a lot of problems).
…s from RetryOperationsInterceptorTests to use vanilla junit assertions.
|
Maybe it's time to bump master to 2.0.0.B-S and use Java8 (default methods)? |
Well, I would consider that in the separate issue: not sure how those default methods can help us here with the |
|
The overloaded methods taking the invocation can be |
|
And all those |
|
The problem with I agree that it looks cleaner from the hierarchy and API perspective, but it is really an abuse of language features when we definitely deal with particular context only and don't care about it in other places. I may miss something else, don't count on me as a final decision stop... |
|
Forget that, it won't work; you need something to hold the invocation. So I guess I'm ok with this. |
|
Are there open issues in the current pull request or can it be merged ? |
|
It is approved on my side and looks like Garry has expressed his OK as well. So, we need only @dsyer 's 👍 since he is a project lead. Thanks for understanding. |
Expose the MethodInvocation in the MethodInvocationRetryCallback in order to permit to concrete RetryListener implementations to inspect in detail the method called as well as its arguments. This information can be used in the retry listener for either detailed logging or monitoring for the proxied method invocations.
This pull-request has emerged based on the discussions held in #119 for the need of exposing detailed information about the AOP intercepted methods on which the spring-retry logic is being applied.