Replace Spring Retry usage to core retry

[snicoll]

This commit replaces Spring Retry by the core retry support introduced in Spring Framework 7. This is a breaking change mostly in configuration that is detailed below.

The main feature in Spring Kafka is BackOffValuesGenerator that generates the required BackOff values upfront. These are then managed by the listener infrastructure and Spring Retry is no longer involved.

Moving this code from BackOffPolicy to BackOff dramatically simplifies that class as Spring Framework's core API naturally provides this information without the need of an extra infrastructure.

From a configuration standpoint, Spring Kafka relies quite heavily on Spring Retry's @Backoff. As there is no equivalent, the annotation has been moved to Spring Kafka proper with the following improvements:

• Harmonized name (@BackOff instead of @Backoff).
• Revisited Javadoc.
• Support for expression evaluation and java.util.Duration format.

The creation of a BackOff instance from the annotation is now isolated in BackOffFactory and the relevant tests have been added. RetryTopicConfigurationBuilder is mostly backward-compatible but uniformRandomBackoff has been deprecated as we feel that its name does not convey what it actually does.

RetryingDeserializer no longer offer a RecoveryCallback but an equivalent function that takes RetryException as an input. This contains the exceptions thrown as well as the number of retry attempts.

The use of BinaryExceptionClassifier has been replaced by the newly introduced ExceptionMatcher that is a copy of the original algorithm with a polished API.

With the migration done, we believe that further improvements can be made here: @BackOff oddly looks like Spring Framework's @Retryable. As a matter of a fact, the maxAttempts and includes/excludes from @RetryableTopic are touching the same concepts. One option would be to open up @Retryable so that it can be used in more case.

Another area of improvement is that harmonization of BackOff as a term. It is named "Backoff" in several places, including in @RetryableTopic, and it would be nice if the concept had the same syntax everywhere.

With Spring Retry being completely removed, this commit also removes the dependency and any further references to it.

[snicoll]

I've just noticed the samples are broken, sample-04 in particular. Not sure how to deal with this given that it doesn't use the snapshot atm.

[artembilan]

Thanks, Stephane!

I'm approving this change since all my feedback is minor, and we can address it on merge as subsequent commit.

We also would need to add your name to all the affected classes.
But a whats-new.adoc deserves respective paragraph to explain such a breaking change.

Let's hear from @sobychacko yet!

[artembilan]

I think we should remove the test for deprecated API even now.

[snicoll]

I kept it as the deprecated code would be dead code and deprecating it doesn't mean nobody would call it. The test makes sure something else does not break it. Or do you mean removing the code and the test?

[artembilan]

Sure! That is just personal preference.

When I see a deprecation warning I assume that code is not supported anymore.
So, I migrate to a new API as soon as possible.

On the other hand, we don't do changes into deprecated code, therefore we may assume that behavior is the same as it was before deprecation.
Hence, removing tests for deprecated API does not change the status quo of that API: even if someone still calls, it really was covered by test before.

I may change my mind, though, if that is really found useful and helpful in other projects to keep those tests just for sake of upstream API heal check.

[snicoll]

It's of course ultimately up to you. I think my point is that not modifying the code and calling it as part of a test are two different things.

The code may not change but what it instruments might. And without the test you won't spot the difference in behaviour.

For something like this one, it doesn't seem very important though.

[snicoll]

Thanks for the review Artem. I've addressed some of that in ec94299 - The only thing I am not sure is about the test testing the deprecated method, see above for more details.

[artembilan]

LGTM.

I'm deferring to @sobychacko for further steps on this PR.

Thanks

[sobychacko]

@snicoll The PR looks good - this is a great effort. As we previously discussed, replacing Spring Retry with the built-in retry efforts in SF 7 seems like the right approach here. The migration feels solid overall from what I can gather.

I like the new isolated approach of BackOff annotation and further isolating its creation via the factory. You alluded to the fact that BackOff looks very similar to SF's @Retryable. Since the current BackOff in Spring Kafka does not contain any Kafka-specific features, are we considering moving it into the Spring Framework proper? Or is that not in the scope? I think this might also benefit other projects, as we would then have a unified BackOff story. Would you have any thoughts on that?

As expected, this is a significant breaking change for the downstream users, especially on configs, annotation switches, or RetryingDeserializer shifting from RecoveryCallback to function with RetryException. Existing projects might have non-trivial work to adjust to these changes. I know @artembilan mentioned adding a section to the whats-new docs, but are we also considering providing migration guidelines with clear examples on how users can adapt to these breaking changes in their applications? It may be possible that people might overlook the new BackOff annotation and wonder what happened to the old Backoff they were used to. On that note, +1 for using BackOff instead of using it interchangeably with Backoff.

Regarding the deprecation of uniformRandomBackoff, I see that you added a note stating there is no alternative in the javadocs. Since it is a public method, we may need to justify the deprecation without providing an alternative. Are there any similar functionalities available in the framework efforts?

Overall, this is cool stuff. The main thing I can think of is the migration docs with quick before/after examples, which will certainly help with a smooth user experience. Once things are merged and settled down, we certainly need to spend some cycles ensuring that there is no regression or gotchas from dropping Spring Retry altogether.

Thank you!

[snicoll]

You alluded to the fact that BackOff looks very similar to SF's @retryable. Since the current BackOff in Spring Kafka does not contain any Kafka-specific features, are we considering moving it into the Spring Framework proper?

No. Doing so would reintroduce the problems we're trying to solve here. Trying to accommodate to slight variations in projects in the core would recreate Spring Retry in the long run.

But here, that's not what I meant. Have you looked at @Retryable? It has all the fields of BackOff plus support for exceptions includex/excludes and max attempts that are in @RetryableTopic.

So here, I am rather advocating whether there's an interest to streamline things in Spring Kafka to adapt to the new annotation in framework, rather than keeping a custom one. That's more efforts that I didn't want to engage in this PR as it could be rather significant. It would require some javadoc change in framework as now it's quite specific to method invocation.

are we also considering providing migration guidelines with clear examples on how users can adapt to these breaking changes in their applications?

Spring Boot has migration guides for new majors. IMO, the only thing that needs explaining is:

• random being replaced by jitter and how to use it.
• The semantic of maxAttempts on a retry policy, and the fact that the total invocations in case of failure will be 4 and not 3 by default.

The rest isn't so bad IMO. The replacement of the calback by a Function is actually adding values. Same for replacing from Spring Retry's callback to a dedicated callback in Spring Kafka.

On that note, +1 for using BackOff instead of using it interchangeably with Backoff.

Can we please consider renaming the field on the annotation? It's still backoff and that looks a bit odd at the moment. I didn't want to take the decision of changing it.

Regarding the deprecation of uniformRandomBackoff, I see that you added a note stating there is no alternative in the javadocs. Since it is a public method, we may need to justify the deprecation without providing an alternative. Are there any similar functionalities available in the framework efforts?

The justification isn't in the javadoc but the name doesn't make sense based on what it does. As for an alternative, just look at the body of the method, it's configuring the jitter which is only using Spring Framework's API and a bit of math :)

We didn't know if this feature is often used. If it is, then I suggest to reintroduce it and use the opportunity to fix the name. You can then have a new method with a more meaningful name being the replacement of the deprecated method.

Thanks for the review!

[sobychacko]

So here, I am rather advocating whether there's an interest to streamline things in Spring Kafka to adapt to the new annotation in framework, rather than keeping a custom one. That's more efforts that I didn't want to engage in this PR as it could be rather significant. It would require some javadoc change in framework as now it's quite specific to method invocation.

Indeed, this sounds like a good idea. This is something we can evaluate to see the impact once we merge this PR.

Good with the migration guidelines in Boot. We can monitor on the Spring Kafka side to see if there is any feedback from the community side, but that is certainly down the road.

Can we please consider renaming the field on the annotation? It's still backoff and that looks a bit odd at the moment. I didn't want to take the decision of changing it.

Sure, I think we can look into it post merge?

We didn't know if this feature is often used. If it is, then I suggest to reintroduce it and use the opportunity to fix the name. You can then have a new method with a more meaningful name being the replacement of the deprecated method.

I don't know how widely this method is used, but the feature of retryable topics via non-blocking retries is pretty popular and may be utilized by many people, so I was just commenting from a public API perspective. I suggest we keep it deprecated and monitor for any potential impacts down the line.

Once again, thanks for this effort!

[artembilan]

Well, looking into the UniformRandomBackOffPolicy in Spring Retry, it feels similar to the ExponentialRandomBackOffPolicy, but without a multiplier.
I would say it should be safe to state this one is deprecated in favor of exponentialBackoff() since we don't carry out UniformRandomBackOffPolicy abstraction anymore.

[sobychacko]

sounds good then.

[snicoll]

Good with the migration guidelines in Boot.

There might be a misunderstanding here. I mentioned Spring Boot has migration guides in major releases. However, we do not intend to document the changes in Spring Boot as it belongs here. We could, however, link to your migration guide from ours. Does that make sense?